pickeduname v5 patch - no version setting #298
Conversation
|
This is mostly LGTM.
|
|
LGTM sounds good until you continue with remarks :-)
* Why move the CTCP VERSION mod to another PR? Do you intend to merge them separately by function? It can be done but more work from me... must I? (please say it can stay as is).
* The bad re notification toast is not obnoxious, it disappears as expected without user intervention, and only appears if there are bad re's, which SHOULD be deleted or corrected by the user SOON, let's remember that if bad re's got in at all, then they got in by user action. I'd leave it as is. It's not even annoying, 2 seconds of hovering text then gone. Adding some sort of once-per-session-connect-by-user-action boolean to quell additional appearances is not impossible, but I'd leave it as is for now.
<aside> I noticed server/channel notices cannot be filtered (yet). This can get obnoxious with the upheaval at freenode/libera now, and I might add that in a future patch, unrelated to the present one. Is anyone else working on filtering/etc code for Revolution IRC?
<aside2> I'd like the chat lib to implement "do not send any CTCP version reply" if any of the 3 string components needed for it are null or "". I do not know if it is so already but I have my eye on that. Would not like to fork chatlib too though.
Answering by email thus no pretty formatting, md does not work via email.
Pe vineri, 21 mai 2021, 16:12:09 EEST, MCMrARM ***@***.***> a scris:
This is mostly LGTM.
* The CTCP option should be moved to another PR.
* The toast with the number of invalid ignore list entries should be removed since it can show at random times when reconnecting to the IRC server in the background, which is not good.
|
Well code-wise this change is okay.
Since I am not sure if we want that feature in the client in the first place and a discussion is needed beforehand. If you want to we can do that now, but in general a better practice is to keep a single PR or commit for a single change.
Why? I do not see any particular reason to rush the user to fix their own garbage, especially in a minor feature like ignore list. This is not stopping them from using the app in any way and should not cause any stability or performance issues.
Not that I am aware of.
It is recommended to compile from sources both of the components when contributing IRC-related changes to Revolution IRC. and create two separate PRs, mentioning that they are related. |
|
Re: single PR per feature: I did not realize this would be a requirement. At this time, I don't feel like separating out the CTCP mod from the re ignore list mod. I have spent way more time getting to speed with github which I have not used previously, than I spent making the changes in the app sources and testing them. If this separation will be needed, then I will help with it when the time comes.
Re: CTCP version needed or not: it is customary to hide the client version and name on some networks, with good reason. The feature comes off by default, and has to be enabled by the user to censor the version and app name.
Re: bad regexp toast on connect(): As I explained, it is not a nagging issue, visually. I tested it several times and it does not get in the way even during reconnects. It appears and disappears, not disruptive. As to why nag user about it, the user needs to input valid data, or edit or delete invalid data in the regexps, as he likes it.
Re: discussion: yes, why not. So far, I have not seen anyone else run the patched app with the new features. Apart from myself. I mean, in the last 3 weeks or so, we talked code formatting, technicalities, but not one user report "it works"? The 2 features I implemented, regexp ignores and CTCP version hiding are important on networks I use.
Speaking of discussion: complexity: why has Revolution IRC reached 225 or so classes and is still growing a bit, albeit slowly? It seems overly complex for what it does. Has a refactoring ever come up in a discussion? Simplifying the app? Not to mention I never saw a single line of javadoc or comment in the code while reversing the source to implement my mods?
|
This is generally the norm for most projects, since it poses a problem where the maintainer has to accept two unrelated changes, and might not agree with one of these.
You are probably running a modern device with no vendor-provided app killer antifeature and have only tested this for a few hours; the toast will show at random unpredicatable times (when the service restarts), unless I missed something during code review or a recent Android change made it impossible to show toasts when the app is not in foreground, which is also possible; in which case it would be a bad practice to rely on this sort of behavior.
There is sadly no testing community on this GitHub reality. There's basically no community. I plan to test the features once the code and scope is sorted out and push any needed fixes. I do not daily drive the app however.
It is what it is. You are significantly underestimating the difficulty of an IRC client; also the number of classes has nothing to do with code quality. Though as I said, the code is not perfect and was mostly written within a 2 month period 2 years ago. I do not use the app anymore and I am not willing to refactor or document the code further; contributions are very welcome however. |
|
Right, I will see what I can do to separate the PR into 2 PRs, one per feature, it is not urgent for me, however. I run the debug version on a device and it's fine. The built debug apk was tested on 4.4 and 8.1 and neither showed problems of obnoxiousness with the bad toast popping up during copious reconnects on freenode and libera recently. I did not check whether the service's toasts are hidden when the parent app is not in the foreground. I think they are issued anyway in the bg too.
|
@MCMrARM I pushed v5 patch with most of your suggestions implemented, with no version code or README.md edits in the push.
I had to wipe the v4 repo, this is a new one, so nothing to revert on v4, just forget it. Some notes, addressing your comments in #296 #297 etc.:
isBadis now an int, as are all former Integers I used.isBadis not transient and is saved to exported zip settings backup, this is useful, it allows people like me to edit the re's in the zip file, seeing better which one is bad.strings.xml. Pluralization was handled using theObjects:syntax which is suggested acceptable by Android documentation on pluralization. Ellipsization was present in the form you mentioned in comments, in the ui.xmlfile, and is not changed by me, so I removed my unsightly ellipsization in the Java code.compilePattern()etc. is redone as you suggested, losing my C-ish code. I refactored a bit and moved the pattern checker intoconfig/ServerConfigData.java.connect(), for the entries for that server alone, and not for all of them. I tested it, it is just a Toast, not obnoxious, not modal, does not slow progress or connection speed. I like it the way it is.The code in the patch is known to build a working debug APK, tested on one device so far, with the added new features.
The patch does not add any warnings to the build process, the build had 100 warnings before patching, and still has as many. None are in sections I added or patched, that I can see.
Commit pickeduname-patch-v5-noversion : 7afa902d591d8d8f463fa4a56801a7cbe97888a4