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

File browser reshuffle 2.0 #11792

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Commodore64user
Copy link
Contributor

@Commodore64user Commodore64user commented May 9, 2024

Original PR here #11781

From the makers of the wildly successful Sleep screen rework #11549 and the yet to be merged status bar makeover #11678, We present to you the 'new kid in town', the new file browser (file cabinet) menu.

There has been a far less aggressive approach here, most of the work pertains the reduction of "view modes" and rewording of settings. Some logic was added for the functioning of the simpler view modes but besides that no changes in functionality were added or removed.

I am conflicted about the use of the word "mosaic" here, as what we get is a standard grid view that has nothing to do with actual mosaics, I would love to hear what you think, should it go, should it stay?

the following screenshots were captured on a non-touch kindle 4, so some options might not appear there. Anyway without any further ado,

Here it is the menu upon opening:

after going to the Library view:

Inside mosaic and list view properties:

and finally, inside the file management submenu:

Important

I would like to have the classic view properties greyed out when 'classic mode' NOT selected but the usual enable function does not work in that case, any help there would be greatly appreciated.

Last but not least, thanks to @Frenzie for his help, patience and support.


This change is Reviewable

@hius07
Copy link
Member

hius07 commented May 10, 2024

Look at it please: you can move

local fc = self.ui.file_chooser

to the top of addToMainMenu, then you will have fc.display_mode_type that is false for classic mode, or "list" or "mosaic".
You can use it to shorten all checks.

@Commodore64user
Copy link
Contributor Author

Commodore64user commented May 10, 2024

to the top of addToMainMenu, then you will have fc.display_mode_type that is false for classic mode, or "list" or "mosaic".
You can use it to shorten all checks.

looking into it

@ryanwwest
Copy link
Contributor

Looks cool. 'Mosaic' has always confused me - I'd be open to a name that reminds you more or a grid instead. In general I think one of the biggest hindrances to new users is settings overload, and while I don't want to remove anything, it's always good to make it easier for users to understand the basics.

@hius07
Copy link
Member

hius07 commented May 11, 2024

First impressions on wordings and placement:
(1) I am not sure that our file browser is a "library". (Collections are closer to "library" or "bookshelf")
"Items" are changed to "books", but they can be (1) folders, (2) books (supported files), (3) files (unsupported).
(2) I'm fine with Classic - Grid - List.
(3) Do not like "File Management" wording.
(4) Do not like additional "tab" in History tab, Collections tab.
(5) "Mix folders and files" has close relation to sorting, I expect them all together.
(6) Is "properties" better than "settings"?
(7) Can we use the same words in: "mosaic view" and "portrait-mosaic mode"?

text = _("Use this mode everywhere"),
text = _("Hide book covers"),
enabled_func = function()
return not fc.display_mode_type == false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually write

return fc.display_mode_type and true or false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about return fc.display_mode_type == false ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's opposite to what you need.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes i know, what i am asking is: would you write that in a different way too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this case it can be
return fc.display_mode_type ~= nil

@hius07
Copy link
Member

hius07 commented May 11, 2024

We will not be able to have, say, Collections in Mosaic with covers and History in Mosaic without covers, right?

@poire-z
Copy link
Contributor

poire-z commented May 11, 2024

(Haven't yet "read" the new wordings.)

@hius07 and me mentionned around #11605 (reply in thread) that we didn't like not having the 6 modes directly available.
Playing with this PR, I find the "3 modes + toggles" not that bad (but not that better :)).

But, with this change, there are issues:

  • Hide book covers and Replace book title with file name do not stick : when you change mode, they are reset
  • This is caused by the fact that you keep using the 6 "modes" values for a single setting in the code - so everytime you change one of the 3 mode/hidecover/usefilename, you need to generate one of the 6 mode values - and you don't store the state of these 3 mode/hidecover/usefilenameanywhere to be able to reuse them.
  • ^ This is what I dislike the most about this PR: the added indirection and gymnastic to build the correct mode in the various menu items callbacks. Before, we had a straight mapping menu item => mode, and it made everything simple and clear. With this PR, we have that added noise and (minor) complexity.
  • Proper fix would be to "migrate" this single mode value to a set of 3 view/hide/use settings - but I'd rather not have us involved in implementing this migration. So, it may need another good idea.
  • Related, causing a decrease in features: one can't set a different hidecover/usefilename for Filebrowser, History and Collections. I guess not many people would use a different hidecover/usefilename for File browser and History - but I guess one could use filename in filebrowser (a real file browser), while wanting to see metadata in history. So, may be if going on with this and the "sticking" is fixed, the History tab> and Collections tab> (I'm also not a fan of "tab") submenu should also get their own [ ] Hide book covers and [ ] Replace book title with file name.
  • (^ I hope you just didn't see that, because if you did, and thought I will just not mention that, if they don't notice it, no one cares, all good, this is not friendly: better mention every change and risk now if you had seen/think them, so we can discuss and find solutions, rather than get surprises later.)

@Commodore64user
Copy link
Contributor Author

  • Hide book covers and Replace book title with file name do not stick : when you change mode, they are reset
  • This is caused by the fact that you keep using the 6 "modes" values for a single setting in the code - so everytime you change one of the 3 mode/hidecover/usefilename, you need to generate one of the 6 mode values - and you don't store the state of these 3 mode/hidecover/usefilenameanywhere to be able to reuse them.
  • ^ This is what I dislike the most about this PR: the added indirection and gymnastic to build the correct mode in the various menu items callbacks. Before, we had a straight mapping menu item => mode, and it made everything simple and clear. With this PR, we have that added noise and (minor) complexity.

you are not wrong there, I know this could have been written in a much nicer way, but I didn't want to be asking for help as I know another approach would be much more difficult given my level of experience (or lack of really)

  • Proper fix would be to "migrate" this single mode value to a set of 3 view/hide/use settings - but I'd rather not have us involved in implementing this migration. So, it may need another good idea.

wink, exactly what I was saying before.

  • Related, causing a decrease in features: one can't set a different hidecover/usefilename for Filebrowser, History and Collections. I guess not many people would use a different hidecover/usefilename for File browser and History - but I guess one could use filename in filebrowser (a real file browser), while wanting to see metadata in history. So, may be if going on with this and the "sticking" is fixed, the History tab> and Collections tab> (I'm also not a fan of "tab") submenu should also get their own [ ] Hide book covers and [ ] Replace book title with file name.

what do you mean? you can certainly still do that, well with the caveat of the [ ] Hide book covers and [ ] Replace book title with file name pointed out. Which leads to me to the last point,

  • (^ I hope you just didn't see that, because if you did, and thought I will just not mention that, if they don't notice it, no one cares, all good, this is not friendly: better mention every change and risk now if you had seen/think them, so we can discuss and find solutions, rather than get surprises later.)

That is quite the accusation there @poire-z, slow down. I am not trying to hide anything from anyone, I think I have been open about issues in the past #11586 (comment). that just went over the radar.

@Commodore64user
Copy link
Contributor Author

(1) I am not sure that our file browser is a "library". (Collections are closer to "library" or "bookshelf") "Items" are changed to "books", but they can be (1) folders, (2) books (supported files), (3) files (unsupported).

do you any other suggestions, otherwise we could revert to display as in "display view"

(2) I'm fine with Classic - Grid - List.

cool.

(3) Do not like "File Management" wording.

again, any other ideas? I was actually quite proud of that one.

(4) Do not like additional "tab" in History tab, Collections tab.

consider it removed.

(5) "Mix folders and files" has close relation to sorting, I expect them all together.

I did think about this, however, this is something you do once therefore probably not needed right at the front of the menu.

(6) Is "properties" better than "settings"?

not per se, my issue lies with many places using "Settings" (not like "x settings") but literally just "settings", I would like to reduce it to just one. I am happy to keep it as "settings" but the "settings" elsewhere would eventually go away.

(7) Can we use the same words in: "mosaic view" and "portrait-mosaic mode"?

that was an overlook, my bad.

We will not be able to have, say, Collections in Mosaic with covers and History in Mosaic without covers, right?

that was already pointed out, as of now the answer would be no. But I don't intend to remove them.

@Frenzie
Copy link
Member

Frenzie commented May 11, 2024

again, any other ideas? I was actually quite proud of that one.

The obvious alternative to just settings is file browser settings.

@Commodore64user
Copy link
Contributor Author

again, any other ideas? I was actually quite proud of that one.

The obvious alternative to just settings is file browser settings.

I don't mind "Settings" being used here, so long as all the other ones slowly disappear -one by one-. Actually it makes some sense to use it here where it is broad and general.

},
{
text = _("Classic mode settings"),
text = _("Classic view properties"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposed change: "Classic view settings".

@hius07
Copy link
Member

hius07 commented May 12, 2024

CircleCI warnings:

plugins/coverbrowser.koplugin/main.lua:96:66: line contains trailing whitespace
plugins/coverbrowser.koplugin/main.lua:102:21: line contains trailing whitespace
plugins/coverbrowser.koplugin/main.lua:151:16: line contains trailing whitespace
plugins/coverbrowser.koplugin/main.lua:471:50: line contains trailing whitespace

@poire-z
Copy link
Contributor

poire-z commented May 16, 2024

  • Hide book covers and Replace book title with file name do not stick : when you change mode, they are reset

I think you could still easily get Hide book covers to stick when switching between Grid & list. But if you switch to Classic, you currently can't know what was the previous state.

  • Related, causing a decrease in features: one can't set a different hidecover/usefilename for Filebrowser, History and Collections. I guess not many people would use a different hidecover/usefilename for File browser and History - but I guess one could use filename in filebrowser (a real file browser), while wanting to see metadata in history. So, may be if going on with this and the "sticking" is fixed, the History tab> and Collections tab> (I'm also not a fan of "tab") submenu should also get their own [ ] Hide book covers and [ ] Replace book title with file name.

what do you mean? you can certainly still do that, well with the caveat of the [ ] Hide book covers and [ ] Replace book title with file name pointed out. Which leads to me to the last point,

I mean just that: the caveat of the [ ] Hide book covers and [ ] Replace book title with file name.

Have you got an idea on how to solve that?
You can't keep using if filemanager_display_mode when handling stuff for History and Collections. Or one would need to go back to filemanager, set Hide covers there, go back 2 submenu down to History to select Grid to get Grid text book cover, then go back to reset it to how he wants it for FileManager. That's too twisted and uglier than the current simple way to handle it.

Your changes are too filebrowser centric. If you want stuff to be intuitive for History and Collections, you may need to triplicate lots of stuff there.

Also, you can't use enabled_func on the Grid size in portrait mosaic mode> depending on only if mosaic is enabled or not for FileBrowser, because then, if you have Grid mode enabled for History, you can't tweak the grid size anymore.
So these settings would need to be always enabled, or triplicated too.

Even if I don't dislike the intended look & feel, I don't think it's worth all these complications :)

@Commodore64user
Copy link
Contributor Author

okay, back here. I have been thinking about this, how to solve the issues and so forth.

I think you could still easily get Hide book covers to stick when switching between Grid & list. But if you switch to Classic, you currently can't know what was the previous state.

I was considering saving to G_settings and reading it.

You can't keep using if filemanager_display_mode when handling stuff for History and Collections. Or one would need to go back to filemanager, set Hide covers there, go back 2 submenu down to History to select Grid to get Grid text book cover, then go back to reset it to how he wants it for FileManager. That's too twisted and uglier than the current simple way to handle it.

I hear you, well read you.

Your changes are too filebrowser centric. If you want stuff to be intuitive for History and Collections, you may need to triplicate lots of stuff there.

lets not.

Also, you can't use enabled_func on the Grid size in portrait mosaic mode> depending on only if mosaic is enabled or not for FileBrowser, because then, if you have Grid mode enabled for History, you can't tweak the grid size anymore.
So these settings would need to be always enabled, or triplicated too.

I hadn't consider this either. that steams mainly from alway using the same view for history and favourites collections. and if I am honest, I also never go to history or use collections so...

Even if I don't dislike the intended look & feel, I don't think it's worth all these complications :)

  • Proper fix would be to "migrate" this single mode value to a set of 3 view/hide/use settings - but I'd rather not have us involved in implementing this migration. So, it may need another good idea.

well it would certainly be easier if you could help. I don't quite get your reticence (see below) so slow steps.

  • Proper fix would be to "migrate" this single mode value to a set of 3 view/hide/use settings - but I'd rather not have us involved in implementing this migration. So, it may need another good idea.

@hius07
Copy link
Member

hius07 commented May 17, 2024

another good idea

Keep the menu structure as is, change the wordings only.

@poire-z
Copy link
Contributor

poire-z commented May 17, 2024

I was considering saving to G_settings and reading it.

That would be an ugly option.
For historical reasons, this plugin saves its settings in the db - at the time, I was strongly suggested to not push crap in G_reader_settings, so I stored them in the plugin db, as it happened it has one. Since then, lots of plugins just don't care and use G_reader_settings, which is fine by me.
But if the plugin should now be allowed to use G_reader_settings, all of its settings should go there - not only little helper settings like the one you need. So, it's another bigger migration. Another lingering idea is to make this plugin part of core, which would allow its metadata db to be of more general use. And that's something bigger, that we can't let to you, and that would require @hius07 or me to have the time and energy to focus on this huge task.
I'm quite against doing "slow little steps", when we have no real time/interest/need/envy/urge to focus on the target situation to be sure these little steps will be the right ones.

well it would certainly be easier if you could help. I don't quite get your reticence (see below) so slow steps.

Above is my reticence: doing things right would need more conviction and interest and work and focus (and time, that I can't afford at the moment), and copiloting you would be painful, for both of us. (I'd rather waste myself a weekend and do it alone myself).
Moreover, even if I don't dislike your menu layout, your enthusiasm does not look like it is really shared :) and migrating settings to a set that fit your model would feel a bit arbitrary. Tomorrow, we could get the visit of AmstradCpcUser that will suggest another menu layout, and there comes a new migration of settings to fit better the new model...
To be honest, your model is quite right: we have 3 individual modules that do classic/grid/list, everything else are parameters to them. But still a lot.

Anyway, some ideas that would not require all the above suggested hard work (I hope I've been scary enough:)), keeping this as a UI only job:

You could store the individual state of hide_cover/use_filename in an internal variable, no need to save it, it would be remembered only for the current menu session. When the menu is first opened and these vars are not there, you would compute their initial values from the initial mode. I think it's fine if we don't remember that when one month ago the use tested Grid with hide covers, and settled on List with covers, if he now switches to Grid he gets covers. You just need that if now when testing stuff, when he has hideCover, when he switches from Grid to List to Classic to Grid, he gets a sticky hideCover.

If you don't want to triplicate things, the big loop that builds the 3 menu items (filebrowser, history, collections) should be reworked a bit, and should also build the Hide cover/Filename only menu items in the 3 submenus - setting the right initial values to the mentionned settings from the proper fb/history/collection current mode.

@Frenzie
Copy link
Member

Frenzie commented May 17, 2024

at the time, I was strongly suggested to not push crap in G_reader_settings

Sorry (probably). :-)

@Commodore64user
Copy link
Contributor Author

Commodore64user commented May 23, 2024

You could store the individual state of hide_cover/use_filename in an internal variable, no need to save it, it would be remembered only for the current menu session. When the menu is first opened and these vars are not there, you would compute their initial values from the initial mode. I think it's fine if we don't remember that when one month ago the use tested Grid with hide covers, and settled on List with covers, if he now switches to Grid he gets covers. You just need that if now when testing stuff, when he has hideCover, when he switches from Grid to List to Classic to Grid, he gets a sticky hideCover.

If you don't want to triplicate things, the big loop that builds the 3 menu items (filebrowser, history, collections) should be reworked a bit, and should also build the Hide cover/Filename only menu items in the 3 submenus - setting the right initial values to the mentioned settings from the proper fb/history/collection current mode.

So I tried to make them "stick" and to adjust the for loops and failed miserably, so two options, three really.

  1. somebody else takes the helm and fixes it (ideal)
  2. change back to the ugly 6 modes (same as before but keeping the wording changes elsewhere)
  3. close this and forget about the whole thing (demolish the house, with the cat and dog inside)

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.

None yet

5 participants