search

itkach / aard2-android

Aard2 for Android, a simple dictionary app
GNU General Public License v3.0
425 stars 98 forks source link

Use a more modern material-designed UI #154

Closed MuntashirAkon closed 1 year ago

MuntashirAkon commented 1 year ago

Also closes #117, #136.

sklart commented 1 year ago

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 commented 1 year ago

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!

sklart commented 1 year ago

I compiled apk (63e8cd8) according to instructions, can try https://drive.google.com/file/d/1cNvwB361E_PBtoGX4pNDpIW-MwaGbBY7/view?usp=share_link

MuntashirAkon commented 1 year ago

@Atrate: I apologise for pinging you here. Could you help me with this three strings by providing many versions? The app crashes on those cases due to missing translations.

Screenshot 2023-03-30 at 18 16 24

Atrate commented 1 year ago

In these cases, many and other pluralizations should be the same I think.

MHBraun commented 1 year ago

I compiled apk (63e8cd8) according to instructions, can try https://drive.google.com/file/d/1cNvwB361E_PBtoGX4pNDpIW-MwaGbBY7/view?usp=share_link

Great. Thank you. I played around with it and I like it. My system is on night mode which is recognized. When searching for an article I can not see my typing. The listing below with the results is black characters in white bubbles. Guess this should be white on black and using no bubbles for seeing more content.

If in article view the tools to manipulate are back on top of the screen. So I have a wonderful menue at the bottom first and then the menu is back on top. This is a little confusing but not an issue.

So far my first 2 cents

MuntashirAkon commented 1 year ago

When searching for an article I can not see my typing. The listing below with the results is black characters in white bubbles. Guess this should be white on black and using no bubbles for seeing more content.

Yes, there appears to be a few issues with ListFragments. I am currently migrating them to use RecyclerViews instead.

If in article view the tools to manipulate are back on top of the screen. So I have a wonderful menue at the bottom first and then the menu is back on top. This is a little confusing but not an issue.

In article view, the pager strip is placed so that the user can see what's on the left- or on the right-hand side. It's simply a suggestion. You can simply swipe to the left/right for the next/previous article. On the initial page, however, the navigation view (at the bottom) is set to make it easy to switch between different features. The three-dots menu are always on top, as usual.

MHBraun commented 1 year ago

When searching for an article I can not see my typing. The listing below with the results is black characters in white bubbles. Guess this should be white on black and using no bubbles for seeing more content.

Yes, there appears to be a few issues with ListFragments. I am currently migrating them to use RecyclerViews instead.

Strange behaviour. Reopening the app the white bubbles are black and I can see my writing.

If in article view the tools to manipulate are back on top of the screen. So I have a wonderful menue at the bottom first and then the menu is back on top. This is a little confusing but not an issue.

In article view, the pager strip is placed so that the user can see what's on the left- or on the right-hand side. It's simply a suggestion. You can simply swipe to the left/right for the next/previous article. On the initial page, however, the navigation view (at the bottom) is set to make it easy to switch between different features. The three-dots menu are always on top, as usual.

Ok. Thanks for clarification.

Keep on going.

francwalter commented 1 year ago

I installed your version a while ago and cannot find anything wrong. The new design is more modern and me too using dark mode where it is possible (old eyes). Thank a lot for your work, it is great that this superb app is getting new programming!

MuntashirAkon commented 1 year ago

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

itkach commented 1 year ago

@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 857a3ace257e64dd1692c4c166dd5a12ac97217e), in no particular order:

image
MuntashirAkon commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago
  • 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Whilst you may be correct with your technical analysis, I still like the approach and the design.

I never understood why Google wants me to put data into my phone on the top of the screen. Usually I am holding and using my phone vertically. Using the apps with my thumbs is impossible as they do not reach the top of the screen. Having said this, I might use a smaller phone, but then I can not read the screen anymore.

Using the phone with the inputs on the buttom of the screen (above the keyboard, if switched on) makes way more sense to me.

Probably there is a way outside Material design or the downsides of Material design may be tweaked...

Thank you for your approach and efforts

Markus

From: Muntashir Al-Islam @.***> Sent: Thursday, April 6, 2023 17:53 To: itkach/aard2-android Cc: MHBraun; Comment Subject: Re: [itkach/aard2-android] Use a more modern material-designed UI (PR #154)

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. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>

francwalter commented 1 year ago

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 commented 1 year ago

Or you do it again, @sklart ?

Compliled (d3a3e41). Can try

francwalter commented 1 year ago

@sklart Thank a lot sklart :)

MHBraun commented 1 year ago

Thank you

Markus

From: Скляров Артем @.***> Sent: Wednesday, April 12, 2023 06:28 To: itkach/aard2-android Cc: MHBraun; Comment Subject: Re: [itkach/aard2-android] Use a more modern material-designed UI (PR #154)

Or you do it again, @sklart ?

Compliled (d3a3e41). Can try — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>

iwouldrathernotusegithub commented 1 month ago

Is there any progress on merging these changes?