search

MCMrARM / revolution-irc

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

apply patch submitted by pickeduname #296

Closed kurahaupo closed 2 years ago

kurahaupo commented 3 years ago

Apply the patch file supplied by pickeduname.

kurahaupo commented 3 years ago

Resolves issue #295

kurahaupo commented 3 years ago

As mentioned in #295 I amended the original patch to reverse several inadvertent changes, in line with that discussion.

pickeduname commented 3 years ago

Ok, @kurahaupo @MCMrARM , I am finally addressing this thread, valuable comments, will be addressed soon in #297 since I "moved" from diff to git. This message could be responded to in #297 or here.

Here are some points, referring to my commit from today, v4, not the one referred to in this thread!:

  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.
  2. 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.
  3. 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.
  4. 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.
  5. isBad and badCount are both Integer but could be int and might become int's.
  6. 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.
  7. 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.
  8. 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).
  9. Noted about refactoring certain function and variable names. I am open to (new) suggestions, now that the code seems to be stabilized and functional. Some functions are candidates for inlining / merging too.

At this point, it would be nice if someone else would also build and test the code as is, to find further bugs if any. After that, I am willing to incorporate the necessary beautification of the code before I'll move on to other projects, hoping for a merge soon. Tia.

Please review the code in #297 commit when commenting, #296 is "obsolete" but we can still talk here, or move the discussion to #297.
Direct link to commit: 612dda3