bksubhuti / tipitaka-pali-reader

A Pali Reading app made in Flutter
21 stars 5 forks source link

Add a Material3 based theme proposal #215

Closed rydmike closed 10 months ago

rydmike commented 10 months ago

Theme proposal and various fixes

Use or abuse the code contributions as you like.

If you want we can do e.g. a video call (if we find a suitable shared time slot) and discuss some of the theme styles proposed here and other changes too. There are so many possibilities and at the end of the day it is just preferences πŸ˜„

The PR contains a commit for each file, so it is easier to see changes per file and a brief comment per file too.

Material 3 theme mode

This PR adds a M3 based theme proposal and style, with color tints on Cards.

It keeps the old M2 based style available, but modernizes it with FlexColorScheme M3 look alike shapes, but keeps the M2 style AppBar and no color tints.

The style included some Card elevation and color fixes on them, that changes the style a bit.

The TabBar for the reader is now in AppBar when there is AppBar and otherwise outside it.

Android - Annotated Region - SystemUiOverlayStyle

An attempt to style the system navigation for Android builds was also added. It required some additional configs with SafeArea to make it looks nice. The AnnotatedRegion and SystemUiOverlayStyle settings only impact Android (phone and tablet), but the SafeAreas might needs double checking on all platforms. I might have missed some layouts I did not know about.

image

Usage of DropdownButton

Consider replacing all usage of DropdownButton, like e.g. lang selection on Settings, with newer widget that supports theming and M3 styling.

The bottom bar on the reader

The custom bottom bar on the reader that can slide away, can potentially also be styled a bit more using the active theme's ColorScheme, to make it fit better with the M3 style, when M3 mode is used. I can look at it later if it is of interest.

Lint and Analyzer Issues

I also fixed a large number of analyzer and lint issues, mostly minor nits. There are however still 33 warnings and 106 hints that it complains about.

Screenshot 2023-10-17 at 05 53 01

There are a lot of lint warning about not "Don't use BuildContext across async gaps". The lint is not always bad, but it can cause crashes in some cases. Some can be solved with unawaited(), while other might require StatefulWidget and checking if context is still mounted.

The "Invalid use of a private type in a public API" are generate when using old format of setting up StatefulWidget.

KNOWN/SEEN BUGS

There are bugs in the navigation. If you use phone and rotate it to landscape and select Setting from rail on the phone in landscape and then rotate phone to portrait the app will crash because it is index 5 (6th one), which does not exist as a selection when rotated back to the bottom NavigationBar.

Other navigation issues seems to be related to used navigation context and animation controller related to animation being disposed. This needs to be studied further.

bksubhuti commented 10 months ago

Yes.. this looks like a massive PR. Did you want me to accept everything and talk later or talk first and then merge. I looked at nearly all the individual comments. I think there will not be any problems.. but I'm not sure if it was complete fixes for all or if I use that and fix other things to match.
I can do a call after lunch in Sri Lanka Time. that would be 12:20pm..best for me. and in my room with wifi. I can also do anytime.. it is my duty to accomodate you. so because it is study week.. i should be studying!! I have the whole day and night free in "reasonable sri lanka times" Lunch stuff is from 11-12:20 including cleanup and personal stuff. The rest is free. The only issue is intermittant electricity problems, but i can go to the study if needed which has solar.

I'm not so great at git.. so my other programmer told me to clone your stuff and test which is easier. https://github.com/rydmike/tipitaka-pali-reader.git

My email is bksubhuti and I use gmail We can use google meet. My viber whatsapp viber is +94767013624 . but I need to replace my sim card which is broken and tied to 2factor security so right now I only have wifi. On linux whatsapp does not do calls. I can do fb messenger too.. bhante subhuti is easy to find.

bksubhuti commented 10 months ago

I tested both android and linux desktop.. The slider m3 bug assert disappeared. (sadhu x 3 as we monks say).. The overal UI looks much nicer. This was such a pleasant suprise to see so much work done for us. I will merge the code. We can talk about this . I was not able to dupicate the bug reliably. I did go to settings and then horizontal (desktop mode) and i did crash the db causing a need for reset. I did again and it survived.

We have a prevention to stop the app from closing on the back button. The desktop of theme is a little less smooth, and causes everything to refresh, but if it fixes the problem.. i'll accept it.

The context lint across async gaps.. not sure how to fix that. Some lint items complain about some variables that are still needed. but we can talk about it. We are very grateful for your unprompted wholesome work (that is the highest level of donation and wholesome mind) .

bksubhuti commented 10 months ago

I did notice that the search filter.. (you have to search for a word.. "try mettā") Then there is a half shown action button for filter.. these results are changed from last build. The filter button is half shown, the top "x" button is half shown .. and there are two buttons at the bottoms (select all and deselect all) which are barely shown. Other than that.. it looks like the transformation is complete. I think that "safe area" as you said will fix that.

The tan reading mode is now an eyesore and needs to change to that light color theme color used in appbar. or a grey. I'll have to change it. I'll also have to remove the hard-coded blue from the chips. But I think we will keep the blue for pull handle to hide or show the bottom tool bar while reading..

The app is working well though on my phone and i'm using as my daily driver. We have many new features coming for our November release, but your PR will be the one feature people will notice and love the most.

Screenshot_20231017-123153 Screenshot_20231017-123145

rydmike commented 10 months ago

I'll have a look at that soon. Not at keyboard right now. Like I said, since I am not familiar with the app there were probably a few place I was not aware of to look at.

Also the rotation bug should be fixable by just not selecting an index on the bottom nav if it is higher than 4, but it might mess with your navigation stack a bit, I can look at it too. I think your navigation stack might have some issues, I saw a bunch of animation asserts that looked related to it.

Yes I also saw the back button prevention handler. That one is always good to have. But of course back should not appear when moving around on top destinations, it did sometimes, I removed it, but that it appeared also indicates there might be a top level navigation issue. For accidental back swipes it is very nice to have though. Matter of taste if you want it on Home, some users like it some not. What I have done in some apps make back go to home, if not on Home instead of exit, and then on home exit, via confirmation and make a config whether it will exit via confirmation on back from Home, so users can configure if you can just keep back swiping to exit the app, or if it will ask for confirmation, when you do so from top Home route. Most apps just silently exit when you do that, so many users expect that.aa default behavior.

Usually in phone mode the Drawer is shown on all top destination screens, not only Home like it is now. Of course on the Dictionary you have the popup guide there now, so there it cannot be added right now unless it is moved.

Never got the app to build on Mac desktop, so I only tested on landscape Android phone and tablet emulator.

Happy to help, I'll get back to you soon with more comments.

Even if you merged it already (wow and cool πŸ˜ŽπŸ™), we could still do a Google video meet anyway just, would be nice to meet you. Plus if you have any questions or would just like to have some other look on the design anywhere discussed, it can certainly be done. I just made some initial quick minor design changes to allow the app to work well in both real M2 mode and M3 mode.

bksubhuti commented 10 months ago

Yes.. would be nice to talk with you. My first exposure to you was watching your interview video on flex themes. You can hear my programming story and why I ordained here . I was an msvc++ programmer in the 90's specializing in DB apps and hardware stuff. I did well and looked really good because nobody else knew the stuff back then. Now it is very common. I ran your code , scanned the changes .. and it was a no-brainer to accept. Later I saw the filter chips which we both missed and I fixed that.

I just pushed some padding to fix the search filter button and the chips. I added padding on it and it worked. I pushed those changes and also set the "medium page color" to the light version of the primary. It looks really nice. Seypia is dead.. looks like an eyesore compared to the beautiful stuff we have now. (btw: even though i take finals.. I'm old.. 53). The app is really nice looking .. we are so grateful for your help!

I'm really weak in git.. so I just push directly as the changes come. Sometimes, I'll push broken code for the Bulgarian programmer to fix.

I'd also like to know what happened to fix the debug assert when you press the slider and go back to settings. . (that is working fine now) I'll fix the ui for the themes settings to be an expansion tile in the next few days and move the page color there too.

I sent a message to Tharindu who is the mac builder and submitter to find out why it does not build. Maybe there are some swift things he does. ven PnDazza (the original and main programmer) is also mac, but he is not doing well and not using computers. I get help from the Bulgarian programmer "sumbodhi" but he is really busy. The app is much more stable than before and hopefully can put to rest. However, a partially implemented firestore plans (not ready for production) will open a feature creep. We are always open for volunteers and very democratic for features and design.
We would love to have a code review and suggestions. Ven Pndazza started this while he was learning, but now he is quite good and does things really well for a self taught programmer (much better than I ever did in the 90s). I also learned flutter recently. My job security as a monk is knowing I'm an old dinasour and cannot program well. But I know enough to do basic things like adding padding.

bksubhuti commented 10 months ago

Tharindu said "Mainly it must be the application ID to something of his development account. But based on the error." If you can, please give the error.. or describe it if you are not near your computer.

bksubhuti commented 10 months ago

I think xcode is needed. The ven pndazza said it worked fine but he refused the upgrade. Perhaps you are using something different. Ven pndazza does not have our app store certificates (neither do I), so it is not dependent on it.

I did push the env file but if you are running Android, you have that settled.

On Tue, Oct 17, 2023, 16:53 Rydmike @.***> wrote:

I'll have a look at that soon. Not at keyboard right now. Like I said, since I am not familiar with the app there were probably a few place I was not aware of to look at.

Also the rotation bug should be fixable by just not selecting an index on the bottom nav if it is higher than 4, but it might mess with your navigation stack a bit, I can look at it too. I think your navigation stack might have some issues, I saw a bunch of animation asserts that looked related to it.

Yes I also saw the back button prevention handler. That one is always good to have. But of course back should not appear when moving around on top destinations, it did sometimes, I removed it, but that it appeared also indicates there might be a top level navigation issue. For accidental back swipes it is very nice to have though. Matter of taste if you want it on Home, some users like it some not. What I have done in some apps make back go to home, if not on Home instead of exit, and then on home exit, via confirmation and make a config whether it will exit via confirmation on back from Home, so users can configure if you can just keep back swiping to exit the app, or if it will ask for confirmation, when you do so from top Home route. Most apps just silently exit when you do that, so many users expect that.aa default behavior.

Usually in phone mode the Drawer is shown on all top destination screens, not only Home like it is now. Of course on the Dictionary you have the popup guide there now, so there it cannot be added right now unless it is moved.

Never got the app to build on Mac desktop, so I only tested on landscape Android phone and tablet emulator.

Happy to help, I'll get back to you soon with more comments.

Even if you merged it already (wow and cool πŸ˜ŽπŸ™), we could still do a Google video meet anyway just, would be nice to meet you. Plus if you have any questions or would just like to have some other look on the design anywhere discussed, it can certainly be done. I just made some initial quick minor design changes to allow the app to work well in both real M2 mode and M3 mode.

β€” Reply to this email directly, view it on GitHub https://github.com/bksubhuti/tipitaka-pali-reader/pull/215#issuecomment-1766212928, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEKXQWHOYKRLOC4IWD4PLK3X7ZTBVAVCNFSM6AAAAAA6DEHWQSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRWGIYTEOJSHA . You are receiving this because you modified the open/close state.Message ID: @.***>

rydmike commented 10 months ago

@bksubhuti, Just as info, I did manage to get the build working on macOS as well, as you suggested deleting the old macos folder and recreating it worked. I don't know if you want me to commit it or not. I did restore its asset icons from the deleted version with git, the rest is pretty much the newest macos folder created with Flutter 3.13.7.

I also looked at the filter padding issues and it looks like you indeed figured that one out.

Screenshot 2023-10-18 at 01 40 08

I have been seeing the problem, saw it already yesterday, where the books disappear. They are there and then suddenly gone. like above on right. They were all then when I started the app, after creating the DB index etc, but are no after going e.g. to Settings. They won't come back unless I delete it it create again, but they always go away after navigating around a bit in the app.

When that happens, all I get is the spinner:

Screenshot 2023-10-18 at 01 43 24

Have you seen that?

rydmike commented 10 months ago

Regarding more issues and any more contributions I do. I will post them as individual smaller issues and tasks in the repo issues first and then make PRs towards them. I can't make any major promises, but sure I can help occasionally, sounds like fun πŸ˜„

I guess I would start with first creating an issue with a overview of topics I suggest I can contribute to, and expand on each one of them with their own issue. Then when I (or anybody) does a PR to fix one, we can reference the issue and close it with a PR.

The massive PR I did was not really good form, but there was at least one commit per file, so that made it a bit easier to see what was happening. Still one issue per PR is the way it should be.

I would probably among those issues add some design proposals with images that explain my suggestions first. Mostly minor things. Ultimately it is all up to you what you think about them and how you want it to look.

Anyway I'll throw in a list first of some topics I've noticed while digging around in the code as well as trying the app.

Overall I'm very impressed by how far this app has come since I first saw it πŸ‘πŸ»

bksubhuti commented 10 months ago

I'm very happy to hear that you can dabble here and there on our project. It is way more advanced than my level. But I sort of know how things work to suggest fixes that I'm not technically capable to perform. I'm mostly widget and package placement programmer with some text and db stuff. I guess that can do a lot. It is sort of like sewing. It is easy .. but as long as the machine works and is tuned correctly. It is always bound to break though.

I know this is bad form, but I'd rather just add you to the project as a contributor. I'm very weak in git, and I need a friend or chatgpt to bail me out when i need to do command line stuff.

I trust you to make the changes, but I don't understand this level.

Android - Annotated Region - SystemUiOverlayStyle
An attempt to style the system navigation for Android builds was also added. It required some additional configs with SafeArea to make it looks nice. The AnnotatedRegion and SystemUiOverlayStyle settings only impact Android (phone and tablet), but the SafeAreas might needs double checking on all platforms. I might have missed some layouts I did not know about.

About the books disappearing .. this is a critical bug that needs to be reliably duplicated. I first saw it only once on my phone recently. The db is somehow corrupted and I'm not sure what is going on. It is concerning. I think it happened when I was testing your issue of trying to make the app crash by going to settings and rotating my phone. (it survived but the books disappeared). There is a reset db in settings/helpaboutetc/reset db . It also helps me locate the db directory when giving support. Perhaps I can open the db and re-compact it and upload again. Maybe it is corrupted.

About the MacOS Build.. I'm glad you got it working with something simple. I will need to rename the git and project directory this November so this process is better. I also just randomly picked a url for our app rather than ensuring I owned it. I don't want to change it, but I will eventually need to. Let me see what flutter version the other macDevs are on. (com.paauk) After that, it would be good to push those files. (The vnpndazza is quite sick so it won't make much difference., and Tharindu, who builds apple stuff for us usually upgrades when I ask). I'll inform him.

The buttons for the filter are fixed (plus I removed the blue color to default) but they are too much padded on desktop as a result.. So, I will put a conditional padding on based on desktop-tablet-view or mobileView. I also fixed the active tab color to match when in the middle-color-page-view (between dark and light). I wonder why m3 changes the layout position.. but I have and can manually tweak it.

One change I'd like to see. I'd like the topnavbar on the search selection to match the beautiful color scheme of the main book categories when in m3 mode. It seems to be a little m2ish.

The rest is really nice. I really enjoy simply looking at the app.

bksubhuti commented 10 months ago

I checked with Tharindu and he is fine with upgrading to 3.13.x . Our project is on his personal laptop. So it is good if you can push the native code for mac (plus other files). You have been given an invite to direct access. You can do PR or direct. Or you can merge paths yourself.

We would not be having these conversations if you were not trustworthy and monk-friendly. You seem to be "anybody-friendly"

bksubhuti commented 10 months ago

About the drawer as a widget. I sort of agree with that. We wanted the dictionary and settings to be available all of the time. You can see that we have dictionary available on the root menu. The dictionary can also be accessed on the pali reader as well by clicking on any word. The settings is on the root menu and also available on the reading page. Making a drawer widget could unify the UI and a consistent place. However, there are other items that need to be where the drawer might go and I think it would conflict. If you have a design especially how to "go back" then it would be good. The lower bottom control bar (during reading mode) is crowded. Moving settings to the drawer could alleviate some of that. I'm quite surprised how quickly you caught on to the app and structure and usage. It took me a long time.. We also don't have all the classes in a logical location and some providers have different naming conventions..

I'd also like to discuss a strategy for removing the context across async. With localization and provider, we get stuck passing context. But perhaps that can be reworked in restructured..

We have a lot of providers running and I think it is getting to be a problem which is another issue. Ven pndazza said he'd use other provider models for his other projects. (he has many projects now).

Localization and Themes were one of my first "tests" and assignments for us to join forces to make TPR. His original app was only Myanmar Language and script. He converted to roman while i did the localization and themes research. That was when you and I first met. I did it with the app Buddhist Sun which was mostly a learning project (proof of concept) for me. That is why I have a sqlite db on it.

bksubhuti commented 10 months ago

I finished my exams today if you wanted to talk. I linted about 100 items. Mostly super.key stuff. But got some of those out of the way.. Sometimes it complains about variables that are not used, but they really are used.. Then we have the big one where the we are using context across async. Not sure how to fix that. Usually many calls require a context. Like the pathprovider and also the localization.

But it would be good to talk to you soon. We have a meeting at 7:30 pm and then I'm mostly free for most days and nights in the next week. We also hope to make a release very soon as well. I want to make the theme a little more spread out .. having it match the general settings and with more labels and cards. I'll try to work on that.

bksubhuti commented 10 months ago

I found this article.. on global keys to help prevent the linting error (besides excluding it :) ) https://medium.com/nerd-for-tech/do-not-use-buildcontext-in-async-gaps-why-and-how-to-handle-flutter-context-correctly-870b924eb42e#:~:text=Solution%201%3A%20Using%20a%20GlobalKey,widget%20is%20disposed%20and%20rebuilt.