Skip to content
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

Use a more modern material-designed UI #154

Closed
wants to merge 46 commits into from

Conversation

MuntashirAkon
Copy link

@MuntashirAkon MuntashirAkon commented Mar 28, 2023

  • Migrated to AndroidX
  • Migrated to Material components
  • Replaced tabs with bottom navigation
  • Implemented auto mode in UI style in addition to light/dark mode
  • Added an option to enable force dark mode in supported webviews
  • Added an option to disable JS in WebView
  • Implement an option to replace/update an existing dictionary (since slob at present does not provide an effective option to update a dictionary)
  • Replaced Font Awesome icons with vector icons
  • Target SDK 33
  • Replaced deprecated Gradle features with alternatives
  • Rewrite Application class to move most functions out of it
  • Cleanup resources, fix lint issues, etc.

Also closes #117, #136.

@sklart
Copy link
Contributor

sklart commented Mar 29, 2023

Changes look interesting. I don’t know how to build apk, is it possible to post app apk to see what the application looks like?

@francwalter
Copy link

Yes, please compile us an apk to test it!
Thanks a lot already!
It is a pleasure to see someone going on with that app, even when the app is already still top usefull and workingly :)
Go on Muntashir!
I will love to test any!

@MuntashirAkon
Copy link
Author

I think the work on the UI and other features are more or less completed, and the PR is ready.

@itkach
Copy link
Owner

itkach commented Apr 6, 2023

@MuntashirAkon Thank you for the PR, there's a lot to like. There are a lot of changes, I need to spend a lot more time with it, but here are some observations/questions so far (I checked out 857a3ac), in no particular order:

  • With Light UI theme, list items occasionally turn dark. It's not clear what exactly triggers it - one time it was after adding some dictionaries, another - after flipping through main screen tabs back and forth few times

image

image

Note that device's system theme is dark, it looks like some list items occasionally "forget" they are supposed to use light theme and switch to system one

  • In Dark theme, at some point UI appeared to cover entire screen, with OS header and footer showing throw but dim. Again, not immediately obvious what triggers this.

image

  • Application logo (used to be in top left corner) no longer appears anywhere. This in itself is not all that important, however logo was also the button to find and show a random article. A bit of an easter egg, but user requested feature and documented.

  • In article view, logo button was a quick way to jump "home" after following links many times as opposed to one by one with Android's back button, now it's not there

  • After updating to you version, I was playing was dictionaries and noticed I couldn't add them back once removed. I had to uninstall the app entirely and re-install it. After re-isntall I was unable to reproduce this.

  • Looks like the base UI theme color is purple - where does that come from? I don't hate it, but I don't love it either. I would prefer system default color scheme and this doesn't seem to be it. Or is it?

  • Bottom toolbar takes up too much vertical space, probably to make room for raised icon with label when selected. Raised button with label doesn't look good to me, I think it would be better without the label.

  • Wording for the new preference item is awkward: "Enable force dark in webview in dark
    mode". Users don't necessarily know what "webview" is and why they may want to enable "force dark". I am not sure exactly what this does, but my guess is that webview now has the ability to automatically make any web content "dark" and this makes it so. Now, most dictionaries already do come with built-in dark theme ("Night") and "Style... > Select Style" offers Default, Night and Auto options, with Auto automatically choosing Night when UI Style in prefs is set to Dark. So this new option is rather confusing.

  • "Disable JavaScript" - what's the rationale behind adding this? It just breaks things - to what end?

  • Code minification - my impression is that enabling it renders stack traces unreadable, additional config is necessary to retain at least ability to map to correct source lines. The app is already tiny (by modern standards anyway). Is there any real benefit to enabling this?

  • Dictionary update functionality that presumably addresses bookmarks may be broken when upgrading dictionnary files #136: I don't think it does. The way I understand it, bookmarks may be broken when upgrading dictionnary files #136 is asking to have a way to convert a broken bookmark to a new lookup. Now, bookmarks typically do not get broken if you update a dictionary - replace it with another dictionary with the same value in uri tag, but if you completely remove that dictionary now bookmark is invalid and there's no way to lookup that word again other than retyping it. This new UI doesn't solve that either. I'm also not sure what the new "dictionary update" button is supposed to achieve. It does open a file and replaces a dictionary, but it doesn't check that the replacement shares the same uri tag, it doesn't check that the replacement is actually a valid dictionary, it doesn't check that the dictionary may already be open (allows duplicates). Can you clarify?

  • New XML icons - where did they come from? Presumably from a source that allows such use and redistribution?

@MuntashirAkon
Copy link
Author

@itkach: Thanks for your review. You should use numbers instead of bullet points as numbers make it easy to address the issues.

Note that device's system theme is dark, it looks like some list items occasionally "forget" they are supposed to use light theme and switch to system one

This is likely due to the use of Context. I'll see what I can do about it.

In Dark theme, at some point UI appeared to cover entire screen, with OS header and footer showing throw but dim. Again, not immediately obvious what triggers this.

This appears to be an issue with activity recreation. While this is supposed to be taken care of by the appcompat library, it wasn't. So, I have to find another way instead.

  • Application logo (used to be in top left corner) no longer appears anywhere. This in itself is not all that important, however logo was also the button to find and show a random article. A bit of an easter egg, but user requested feature and documented.
  • In article view, logo button was a quick way to jump "home" after following links many times as opposed to one by one with Android's back button, now it's not there

Added.

  • After updating to you version, I was playing was dictionaries and noticed I couldn't add them back once removed. I had to uninstall the app entirely and re-install it. After re-isntall I was unable to reproduce this.

Not sure why it happened. I'm unable to reproduce this right now. I'll fix it if I can.

  • Looks like the base UI theme color is purple - where does that come from? I don't hate it, but I don't love it either. I would prefer system default color scheme and this doesn't seem to be it. Or is it?
  • Bottom toolbar takes up too much vertical space, probably to make room for raised icon with label when selected. Raised button with label doesn't look good to me, I think it would be better without the label.

I'm guessing you're not familiar with Material Design. Visit https://m3.material.io/ to get a better idea. The title of the PR says material design. So, addition of navigation components from M3 library should be expected. But, I can disable the label like it was before. But modern phones have a large vertical space, and a lot of apps use the same BottomNavigationView without issues. We try to use standard components because they have better accessibility features than the custom ones and reduce the pain of thinking about accessibility.

  • Wording for the new preference item is awkward: "Enable force dark in webview in dark
    mode". Users don't necessarily know what "webview" is and why they may want to enable "force dark". I am not sure exactly what this does, but my guess is that webview now has the ability to automatically make any web content "dark" and this makes it so. Now, most dictionaries already do come with built-in dark theme ("Night") and "Style... > Select Style" offers Default, Night and Auto options, with Auto automatically choosing Night when UI Style in prefs is set to Dark. So this new option is rather confusing.

Yes. But this feature is only enabled for WebViews that support it. “most dictionaries already do come with built-in dark theme” is a wrong assumption since slob format has no standards for setting custom styles. It rather infers from the files which is a hack than an actual feature. All the dictionaries I use are converted using pyglossary or created by myself and they do not contain such non-standard features. Also, injecting night styles this way (using JS) is no longer needed with CSS3. Therefore, the function is not really confusing to those who're familiar with it. But, of course, the language can be altered or explanations can be added.

  • "Disable JavaScript" - what's the rationale behind adding this? It just breaks things - to what end?

It doesn't break things for dictionaries that do not require JS, and all of my dictionaries do not require any JS. There are a few reasons:

  1. The server does not have authentication meaning other apps with the INTERNET permission can access the server. This is a vulnerability that can be reduced by disabling JS in the WebView. Another option is to enable authentication in slobber. But since it's a separate library and other people may rely on it, I didn't attempt to modify it.
  2. Some people like me simply dislikes JS in a browser. I use most of the sites with JS disabled by default.

The function is disabled by default with a description. So, it should be enabled by only those who are like me (and there are a lot of F-Droid users who would agree with me).

  • Code minification - my impression is that enabling it renders stack traces unreadable, additional config is necessary to retain at least ability to map to correct source lines. The app is already tiny (by modern standards anyway). Is there any real benefit to enabling this?

You can disable it if you want. But to be quite frank, I know many people who care about storage space. For example, some people refrained from downloading Openreads after it migrated to Flutter which increased the file size by 8 folds. I have replaced some apps by creating slob files from their database because they take too much space. My observation was that with compression enabled, a slob file takes much less space than a SQLite3 database even with all the HTML tags included. With the inclusion of libraries with large and useless dependencies such as appcompat and material-components, it saves a lot of space. With minification enabled, the app takes consumes around 3.39 MB in its compressed form. You can upload the map file along with the APK in GitHub releases and may be disable it for F-Droid builds.

  • Dictionary update functionality that presumably addresses bookmarks may be broken when upgrading dictionnary files #136: I don't think it does. The way I understand it, bookmarks may be broken when upgrading dictionnary files #136 is asking to have a way to convert a broken bookmark to a new lookup. Now, bookmarks typically do not get broken if you update a dictionary - replace it with another dictionary with the same value in uri tag, but if you completely remove that dictionary now bookmark is invalid and there's no way to lookup that word again other than retyping it. This new UI doesn't solve that either. I'm also not sure what the new "dictionary update" button is supposed to achieve. It does open a file and replaces a dictionary, but it doesn't check that the replacement shares the same uri tag, it doesn't check that the replacement is actually a valid dictionary, it doesn't check that the dictionary may already be open (allows duplicates). Can you clarify?

uri, again, is neither documented nor a required tag in slob, and quite frankly, you cannot expect all the dictionaries to contain the tag since not all of them are online dictionaries which seems to be your basic assumption here (e.g. Wikipedia). So, a better replacement method is needed. So, if you used the replace button, it would rewrite history to alter the slob IDs, not URIs. Getting a slob ID does require a valid dictionary. The duplicate issue has now been fixed. If you think that it's necessary to explicitly handle URIs, make them a part of the standard so that we know what it exactly does. But I think a better way would be to handle versioning within slob by allowing a unique ID (instead of new UUIDs) and version code/name as mandatory tags.

@MuntashirAkon
Copy link
Author

Dark mode issues should be fixed now.

  • After updating to you version, I was playing was dictionaries and noticed I couldn't add them back once removed. I had to uninstall the app entirely and re-install it. After re-isntall I was unable to reproduce this.

I was able to reproduce an issue where altering the configuration of the app (orientation, locale, dark mode, etc.) caused this issue which I've now fixed. I don't if it's the same issue.

@MuntashirAkon
Copy link
Author

  • With Light UI theme, list items occasionally turn dark. It's not clear what exactly triggers it - one time it was after adding some dictionaries, another - after flipping through main screen tabs back and forth few times

Now, only this issue remains, it seems.

@itkach
Copy link
Owner

itkach commented Apr 6, 2023

I'm guessing you're not familiar with Material Design. Visit https://m3.material.io/ to get a better idea. The title of the PR says material design. So, addition of navigation components from M3 library should be expected. But, I can disable the label like it was before. But modern phones have a large vertical space, and a lot of apps use the same BottomNavigationView without issues. We try to use standard components because they have better accessibility features than the custom ones and reduce the pain of thinking about accessibility.

Modern phones are also narrow, so when you hold them the other way they actually have very little vertical space. Nobody is arguing against using standard components, but they can still be used with different options. In addition to using more vertical space, label appearing only for active button makes UI look jumpy. Take a look at, say Google's Play Store or Maps - nav bar at the bottom, all the labels are on all the time.

But, I can disable the label like it was before.

Also, the name of the screen was displayed in the subtitle. Could be just the title, showing name of the app itself is probably not very interesting.

Yes. But this feature is only enabled for WebViews that support it. “most dictionaries already do come with built-in dark theme” is a wrong assumption since slob format has no standards for setting custom styles.

It's not an assumption. wikipedia/wiktionary, gcide, wordnet, freedict - all do. You're right in that there is no standard prescribed by slob. Slob is a binary storage format and doesn't mandate how applications interpret the content or tags. applications are free to make up their own conventions. This application happens to support alternative style sheets and many dictionary converters choose to include alternative style this way.

It rather infers from the files which is a hack than an actual feature.

total hack, agreed :) served users pretty well though

Also, injecting night styles this way (using JS) is no longer needed with CSS3.

Supporting dark appearance is not the point though. The point is that dictionary itself can come with multiple appearances. Injecting css using JS is also used for user styles.

Therefore, the function is not really confusing to those who're familiar with it.

Who would that be? :)

But, of course, the language can be altered or explanations can be added.

Don't get me wrong - automatic dark theme is cool, the way it interacts with the existing style selection feature is not good. What if instead of switch in prefs it would be another option in Style... selection.

It doesn't break things for dictionaries that do not require JS, and all of my dictionaries do not require any JS.

It is pointless to disable Javascript for dictionaries that simple do not include any.

The server does not have authentication meaning other apps with the INTERNET permission can access the server. This is a vulnerability that can be reduced by disabling JS in the WebView.

Can you elaborate? What's the hypothetical attack scenario here? And how disabling JS in this application is related to other applications?

Some people like me simply dislikes JS in a browser. I use most of the sites with JS disabled by default.

This is not a web browser though. Users do not log in to web sites withing this webview, do not open external links, do not fill forms.

My observation was that with compression enabled, a slob file takes much less space than a SQLite3 database even with all the HTML tags included.

I am not sure I understand what you are talking about ..slob files - the dictionaries - have nothing to do with application code, and compression is part of slob format definition, but again - nothing to do with Android apps.

With the inclusion of libraries with large and useless dependencies such as appcompat and material-components, it saves a lot of space.

Aren't libraries already minified?

With minification enabled, the app takes consumes around 3.39 MB in its compressed form.

Last release .apk is 3.31MB, without minification. How big is release apk for your version without minification?

@itkach
Copy link
Owner

itkach commented Apr 6, 2023

uri, again, is neither documented nor a required tag in slob,

As said above, slob-the-binary-format does not dictate how applications use tags. This application makes up some conventions. label tag is another such convention. If these tags are not present, the app still works (sans the features enabled by these tags) and is still able to use dictionaries.

you cannot expect all the dictionaries to contain the tag since not all of them are online dictionaries which seems to be your basic assumption here (e.g. Wikipedia).

That is not the assumption here at all. URI is "universal resource identifier", not a URL (universal resource locator), although URLs are also URIs. uri is simply a marker used to identify related content, indicates same "logical" dictionary if you will. This is not just for "versions" (well, not really for versions) , this is how multi-volume dictionaries are tied together and how additional (optional) content may be provided (e.g. images).

So, a better replacement method is needed.

Is it? Maybe so, but what's in this PR ain't it. A good start would be to articulate what problem exactly are we trying to solve with this and then offer a solution in a separate PR.

@MuntashirAkon
Copy link
Author

Modern phones are also narrow, so when you hold them the other way they actually have very little vertical space.

In that case, using Material Design is pointless because it adds spaces everywhere. For example, the lists consume almost twice the space than it used to. So, with this argument, it can be concluded that the entire UI design is faulty and inaccessible, thereby, not mergeable. However, in my phone, the old tabbed UI consumed about 8 mm and the new navigation view consumes about 13 mm. How a 5 mm difference makes such difference does not make any sense to me.

In addition to using more vertical space, label appearing only for active button makes UI look jumpy. Take a look at, say Google's Play Store or Maps - nav bar at the bottom, all the labels are on all the time.

Unfortunately, I don't use those apps. But if you want the labels to be displayed all the time, that can be arranged. It's not a big of a deal either.

It's not an assumption. wikipedia/wiktionary, gcide, wordnet, freedict - all do. You're right in that there is no standard prescribed by slob. Slob is a binary storage format and doesn't mandate how applications interpret the content or tags. applications are free to make up their own conventions. This application happens to support alternative style sheets and many dictionary converters choose to include alternative style this way.

As said above, slob-the-binary-format does not dictate how applications use tags. This application makes up some conventions. label tag is another such convention. If these tags are not present, the app still works (sans the features enabled by these tags) and is still able to use dictionaries.

That is not the assumption here at all. URI is "universal resource identifier", not a URL (universal resource locator), although URLs are also URIs. uri is simply a marker used to identify related content, indicates same "logical" dictionary if you will. This is not just for "versions" (well, not really for versions) , this is how multi-volume dictionaries are tied together and how additional (optional) content may be provided (e.g. images).

I see. I assumed this to be a client capable of parsing slob where slob itself is the standard. Based on that assumption, I basically used the app to replace other apps that simply display contents stored in a database, in order to reduce the number of apps I use in my low end phone, and thought that it would be amusing if I can polish it a bit by contributing to the app. But, from what you said, it appears that my assumption was wrong, and I mistook it for something that it isn't. So, probably, this PR should be closed because all my work are based on that single assumption. It's obvious that you didn't like the changes, and the audiences may not actually prefer this scenario.

@francwalter
Copy link

I like the new design and switched to that apk already (thanks for compiling, @sklart ).
What I like too (and even more) is that somebody did so much work to continue this great app (thanks for it @itkach !!!) which I use since years and often.
This is "the spirit of open source" and I would like that you would continue :)
... and compile the latest commits ;)
Thanks a lot!

@MHBraun
Copy link
Contributor

MHBraun commented Apr 9, 2023 via email

@francwalter
Copy link

I am with you there, Markus, thanks for pointing that, me too I always use my phone always portrait. I even programmed buttons at the bottom to better reach it, in a little (Tasker) script-app I made ;)
@MuntashirAkon I appreciate your code, if you discontinue now, as these many PRs are neglected, could you create at least an apk from it?
Or you do it again, @sklart ?
I think I could do it as well, but it would take long for me, the first time, much longer than you :)
I never compiled the aard2 app yet.
Thank!

@sklart
Copy link
Contributor

sklart commented Apr 12, 2023

Or you do it again, @sklart ?

Compliled (d3a3e41). Can try

@francwalter
Copy link

@sklart Thank a lot sklart :)

@MHBraun
Copy link
Contributor

MHBraun commented Apr 21, 2023 via email

@ghost
Copy link

ghost commented May 14, 2024

Is there any progress on merging these changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Light mode Navbar in Dark Mode
6 participants