-
Notifications
You must be signed in to change notification settings - Fork 163
Modernize #285
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
base: master
Are you sure you want to change the base?
Modernize #285
Conversation
|
Thanks a lot for your contribution as usual! |
.gitmodules
Outdated
| @@ -0,0 +1,9 @@ | |||
| [submodule "git-diff-image"] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the purpose of these submodules?
.github/workflows/Android-CI.yml
Outdated
| id | ||
| - name: prepare | ||
| run: | | ||
| sudo apt-get update && sudo apt-get install -y exiftool imagemagick xdg-utils libimage-exiftool-perl zsh jq xorg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where are all these binaries used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for later applied screenshot compare Espresso tests.
When I remove it, then it's clear that screenshot compare Espresso tests will never come
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a copy of AppDevNext/AndroidChart#278 (comment)
But I guess it's better to remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this
|
code changes looks good and i'm fine, but do you mind just reviewing those actions and clean them up? no commented code and trim them down to the bare minimum i.e. build & test + release, or just add some comments about all the other binaries installed and the submodules dependencies also afik the ndk never worked properly or it's not needed, isn't it? if you agree could you please simply delete all that unnecessary code and keep it as simple as possible. wdyt? we could do a 3.x release without all the jni code i also think that rx-java could be replaced by kotlin native coroutines, but i'll leave that to you |
|
bottom line, i don't have any availability to work on it other then reviewing PRs and any improvement should be towards maintenability and stability, the simpler the better and i do really appreciate your help 😄 |
It caused a compatibility warning, it was the main reason, why I touched it |
please double check, but IF it works without it and it's not required just nuke it, you have my blessing - it's been a pain in the neck since the beginning and if not mistaken the idea was to have 2 implementations if the kotlin code works there is no need anymore for the other impl, it has to be maintenable |
Sure, it's a pain and a unneeded complexity
it it's not needed, I'm fine to remove it. But I want not to touch code on reason "because". It would need tests (I've no motivation), manual or my Espresso (here it is screenshot compare again) My current knowledge is, that it's currently the minimum |
|
i would prefer a 3.x release if we remove the native code because it's a breaking change, but i think that the single entry point is this line, that was the original idea to test it i would just leave it |
|
did you test the pr somehow and is it safe to be merged? |
|
To be honest: no |
|
please let me know if i can help somehow in the review or you want to take some time, just ping me when you think it's ready to be merged and sure it's still working. thanks again for your help |
|
I guess I'm ready and when you want to help, please do a manual test |
I see there is still java code. This could be converted as well |


Please see details in commit messages