-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
updated app to androidx / material and feature improvements #184
Comments
If your changes become huge, those won't be merged into the official repo. |
BTW, there have been several huge forks with full rework of this app (but I suppose those are stale now) — you may find cool things to rebase on or improve on. |
@soshial Nice did not know. Will take a look |
@farfromrefug thank you, I appreciate the effort. So far it looks mergeable. Even though it is technically quite big in terms of number of changes, most of these changes appear to be rather mechanical/cosmetic, nothing changes conceptually. I do see, however, some changes unrelated to androidx migration already crept in: for example replacing switch statements with ifs here and here and in other places, or an attempt at issue fix, or merging a pull request, or tooling updates. While such changes are not necessarily blockers (and some may even be unavoidable, like tooling updates aggressively forced by Google), they do make it more difficult to review and test, and may make it impossible to merge if enough of them are accumulated in one PR.
If that's what you would like to do (full rework) then I think the best course of action would be to keep that as a separate project and release it under a new name. I'd be happy to recommend users check out such a new application when it is ready.
I achieved my personal goals I had with this project a long time ago. I do keep using it myself (lightly) and I would like to keep it in working order for the foreseeable future if I can, but I do not plan or want any new major development. Small fixes and tweaks, occasionally catching up with Android tooling, refreshing the look/theme - sure. Re-architecting the app, re-thinking navigation patterns and UI in general, re-organizing the code, re-writing in Kotlin (as someone suggested once) - all these may be great ideas potentially resulting in great new app(s) but I think they are best pursued as their own separate projects. |
@itkach switch migration to if is needed because it is not supported anymore in latest java. As for the future and big updates like architecture, navigation,..., i am willing (if it is something i need as my personal need for this app) to implement them. If you agree you could add me as to this repo and i could implement such things with PRs so that you can still have a look at it |
Hm... that would be a pretty incredible breaking change in Java, and indeed I can't find any documentation that would support your statement. Can you elaborate? |
You can read more about problems with switch statements here. Basically it won't compile if the integer is not final: |
Ah, so switch is supported in latest Java just fine - phew, got me worried there for a moment! 😌 - this is android tooling issue, thank you for clarifying. Looks like, more specifically, it's a breaking change in Android Gradle Plugin 8.0.0 defaults. Since this defaults change doesn't benefit this app in any meaningful way, an easy way to avoid changing the switch statements would be to explicitly set |
|
You mean
Why not? |
Updated my answer |
@itkach i think upgrading to gradle 8 is kind of a must have. Anyway at one point you will be force too to be compatible with latest targets and thus release with new features. But anyway it is your choice, just dont see the issue in migrating to if |
It optimizes something that this application doesn't need optimized while forcing unnecessary code changes. This is a misguided default change - which is par for the course for Android. Anyway, this conversation started with you saying "switch migration to if is needed ..." and I'm just pointing out that's not the case, that's all. |
@itkach yes sure dont worry I ll create à PR for what you accept and the rest will remain in my fork. Might just come a point where I cant PR anymore as it does not align with my goals. I am still "comparing" your slob format and zim. |
BTW, @farfromrefug, if you maintain this app (or its fork), I'll come in and help from time to time with some features/bugfixes/improvements, because I use this app really often. |
@soshial thanks a lot. Yes i think i might have to go for a work. I am already making more changes for better usability and i have more ideas. |
BTW, since you're migrating to Material Theme, @farfromrefug, it would be cool to migrate the tabs as well:
|
@soshial Yes i plan on going full material. Just gonna take some time as this mean true rewrite not just theme / style migration |
@soshial here you go the app is now using kotlin and using material components: |
Wow, this looks pretty much amazing, @farfromrefug! 🔥 |
@soshial i have no idea :D Materiaard ? :P |
Just so it is known and to prevent someone starting this. I started working on upgrading the project and improving UX / features.
The work is happening here https://github.com/farfromrefug/aard2-android
Here are a few screenshots
The text was updated successfully, but these errors were encountered: