search

MCMrARM / revolution-irc

A modern Android IRC client. #revolutionirc on Libera.chat
GNU General Public License v3.0
553 stars 63 forks source link

pickeduname patch cumulative v4 - 1st git commit from diff-v4 #297

Closed pickeduname closed 3 years ago

pickeduname commented 3 years ago

v4 deleted, v5 is new and only patch now, see #298

this issue remains unresolved: @MCMrARM :

While editing the v5 patch I found that AS deletes two lines from config/ServerConfigData.java, which are:

Am unable to explain why it does this. Did that in AS 4.0 and also now in upgraded AS 4.2

MCMrARM commented 3 years ago

Let me reply to the notes from the other PR:

  1. reverting version numbers and text in README.md and app/build.gradle can be done but I prefer to keep those as I pasted them. A merge-clean branch with these reverted will be made if requested. But I usually bump versionCode and Name liberally to keep changes visible. Please never ever decrement version numbers for any reason. Reverting "my" version number used for debugging is perfectly fine on merge, of course.

as I said, it's my responsibility to increment the version number with the next release; PR have no relationship with releases so this is the wrong place to do an increment; README.md is the project readme; the notes about what the PR changes should be in the PR description

  1. bracketing re's with // globs with ?? or with no brackets (default), seems to work well. The hint strings in the Ignore Entry screen have been edited so they show Any /regexp/ or ?glob?, i.e. no surprise to the user.

This is okay for now, althrough not the best UX

  1. the way pattCompile() is written is the C way, as I intend to use the code elsewhere too. The goal is to set up the call to the real pattern compile function only once, since that is the expensive call usually. The int j is used to offset / cut the head and tail of the string, thus values 0 or 1 and it is not a boolean for the same reason. Could have named it i for index, j is also used as an index integer frequently. Initially I used i for isRe but then renamed it and it became a boolean. This is how it stayed j and definitely int. The way @MCMrARM coded it in #296 is a more Java esque idiom and terser, I think his version is better, for Java. Similarly, the compile() function name is imo not reflecting the true nature of the function. But it could be reverted, or ... see 9. below.

The project is Java. Easy code reuse is no excuse not to follow a language's idioms.

  1. isBad is not transient and is saved in the .zip exported settings and in persistent settings. Note there's a transient badCount too, which is used to pass the count of bad entries from the compile / check function back up to the caller which can raise the toast. badCount counts the number of set isBad flags / entries. isBad is used to mark the Ignore List entries @bad: in the ListAdapter among other things, which could (should?) be available for edit before connecting to a server, but isn't currently so, hope it will be some day. This means that the list adapter is able to display the updated marked bad re containing entries without calling the check/compile function itself. Note that the syntax check is triggered from 2 places now: 1) from connect() and 2) from exit from an Ignore Entry edit. The latter only checks the current screen's re's, not all, the former checks all. Without a persistent isBad, it would have been necessary for the list adapter to call the compile / check function.

I think it should be fine to force recompilation of bad regexes every app launch though, since we already need to recompile the valid regexes and it's pointless to optimize for an error case. I would mark isBad as volatile here.

  1. isBad and badCount are both Integer but could be int and might become int's.

Do not use Integer if possible since that's a boxed type, always use int unless you can't.

  1. Ellipsizing the Ignore Entry text is indeed not using the facilities provided by Android but I would like there to be at least a few characters of the text match pattern in the line, no matter how small the screen is. I have devices with very small screens.

Post a screenshot illustrating this issue; either way no, trimming the text in code is not okay. I do not think this issue would be limited to this screen only though.

  1. Similarly, the count of bad re's is not pluralized properly using the mechanisms provided by Android strings.xml etc., this is seen in app/src/main/java/io/mrarm/irc/ServerConnectionInfo.java @~L153.

I don't think displaying the count on app startup helps the user in any way; I think it's just adding unneeded noise and nagging. This will also possibly display on transparent app restarts in the background; I think it's best not to bother the user with an unneeded check on app startup since they already have got the error when editing the regex.

  1. re: app/src/main/java/io/mrarm/irc/EditIgnoreEntryActivity.java @~L95: Indeed that toast could use Android strings and with that also the pluralization mechanism (6. above).

This should only display an error related to the currently editing entry removing the need for a plural string.

pickeduname commented 3 years ago

Re: code changes: will have to wait for v5 patch, Sunday probably. @MCMrARM Your comments are pertinent and I reacted to them, in v5, which you have not seen yet (Sunday hopefully).

All the issues you brought up in #296 were addressed, strings, etc. There are 2 or 3 things I left as is for now but may be done by Sunday. One is ellipsization (sp?). All strings are in strings.xml and I refactored / moved classes as you suggested, moved the strange decision code in the regexp/glob compiler to your version suggested in #296 etc.

I will revert the versioning for the branch for which the pull request will go out (v5), but I need to keep my own branch on different version numbers else I have trouble keeping track of on-device installed versions.

We'll see by Sunday night (my time is Eastern EU TZ).