some maintenance and code readability #279
Conversation
TacoTheDank
commented
Nov 24, 2018
TacoTheDank
commented
- changed some res layouts for very slightly better performance
- escaped apostrophes that were causing errors
- java 1.8 compatibility (converted anonymous java expressions to lambdas and methods, etc)
- make build.gradle files a bit more understandable
- update kotlin to 1.3
- update gradle to 4.10.2
There was a problem hiding this comment.
Well, that's a big pull request! But I can see you pretty much modernized the code base and applied (a lot) of lints, which isn't a bad thing, as well as reordered quite a few XML attributes. I'm not sure if I'm happy about the HTML changes, as in my opinion it reads worse now. Thanks for the updates to the translations as well! I would have also preferred… smaller commits with good messages, but that's alright.
Please address my other comments before we get any further.
| @@ -25,6 +25,6 @@ public boolean isConnectedToInternet(@Nullable @StringRes Integer warnMessageStr | |||
| if (!result && warnMessageStringRes != null) | |||
| Toast.makeText(_context, _context.getString(warnMessageStringRes), Toast.LENGTH_SHORT).show(); | |||
|
|
|||
| return result; | |||
| return !result; | |||
There was a problem hiding this comment.
Considering this function now does literally the opposite, shouldn't we rename it as well? Maybe we can rename it to isOffline directly. Much shorter.
There was a problem hiding this comment.
I feel like I should probably leave that to you :/ but I'll do that anyway
There was a problem hiding this comment.
Well, the change is yours, and keeping the function name correct is relevant with this set of changes. But I could do it in your repository before merging if you really don't want to do it yourself.
Edit: Oh you changed it. It's good now, thanks.
| } else { | ||
| return null; | ||
| if (i >= 0) { | ||
| foundIcons.size(); |
There was a problem hiding this comment.
This is a no-op, which I'm not sure what the purpose is. I think you should revert this part.
| @@ -88,8 +88,8 @@ public int compareTo(final ResTag o) { | |||
| //region De/sanitize Content | |||
|
|
|||
| private static boolean isEscapeSequence(char which) { | |||
| for (int i = 0; i < ESCAPE_SEQUENCES.length; ++i) { | |||
| if (ESCAPE_SEQUENCES[i] == which) { | |||
| for (char ESCAPE_SEQUENCE : ESCAPE_SEQUENCES) { | |||
There was a problem hiding this comment.
While we're at it, ESCAPE_SEQUENCE should be escapeSequence, really. This isn't a constant.
| @@ -1,25 +1,43 @@ | |||
| #!/usr/bin/env bash | |||
| #!/usr/bin/env sh | |||
There was a problem hiding this comment.
Not sure if it's a good idea to modify this file since it's a gradle thing.
There was a problem hiding this comment.
It doesn't affect anything in itself. It's merely using git bash and running "./gradlew wrapper --gradle-version 4.10.2 --distribution-type all", basically keeping all gradle(w) files at the correct version with each other.
|
Lol I need to remember to make my commit messages a little more verbose. I first group them by directory and module, and add them to different changelists named after said groupings. I then commit each changelist. |
Signed-off-by: TacoTheDank <[email protected]>
|
Looks good! Need to test it before merging. |
|
You can always revert the HTML changes if you don't like them :) (To avoid having extra commits, I think one can pull up to that commit, revert said undesired change, and then pull the remaining ones. Has to be done through git bash (or Android Studio if you prefer) though.) |
