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

set global settings independent of local setting #5522

Merged
merged 10 commits into from Oct 25, 2019

Conversation

@yparitcher
Copy link
Contributor

yparitcher commented Oct 23, 2019

allow setting the global inverse_reading_order regardless of what the value is for the current document.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Oct 23, 2019

It's by design. The idea is:

  1. I like this value.
  2. Let's make it default.

There are some others that are written the same way (e.g., Hanging punctuation).

I'm inclined to think making it independent of the displayed setting adds a lot of extra holds & taps to achieve the same result.

Pinging @poire-z

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 23, 2019

Was going to approve, seeing it does what's promised in this PR text :)
But I never really looked at how that work. I might have set one or two as default once and never looked again.

So, our current behaviour is that the "default" button is not a toggle? When you hit it, it just sets as default the current value? I guess that looks fine.
Might help then that the "default" button would be grey/disabled when the default is already the current value (so, you know it applied when you hit it by becoming grey - or that you need to do something else to have it enabled, like hitting the main OK/Cancel.)
edit: oh, but it's not a button ! (got confused by #5497 (comment))

Anyway, no real opinion - both our current behaviour and @yparitcher one might be not really obvious. But a single global same unobviousness is better than multiple :)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 23, 2019

OK, so it wasn't a button :) discard my previous middle paragraph...

Well, for font & hyphenation, so lists, we set a symbol for the default one. For other menu item with a checkbox, no real way or reason to do the same (it would add noise to the menu).
But may be just a clearer dialog, saying what is the current value, and (instead of Cancel|OK) 2 buttons: Set Disable as default | Set Enable as default would make all that clearer?
There woule be no real way to escape that dialog (except hitting outside), so you'd have to make a choice.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Oct 23, 2019

It's an option, but as a user I'd feel better with a dialog like this:

Screenshot_20191023_101416

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 23, 2019

Oh, of course, we can do 3 buttons :)

@yparitcher

This comment has been minimized.

Copy link
Contributor Author

yparitcher commented Oct 23, 2019

It's by design. The idea is:

1. I like this value.

2. Let's make it default.

There are some others that are written the same way (e.g., Hanging punctuation).

I'm inclined to think making it independent of the displayed setting adds a lot of extra holds & taps to achieve the same result.

i see where you are coming from, however i bumped into this when trying to disable the global while the local was enabled, and kept on getting the enable dialog.
At first i thought there was a typo or bug and was confused until I read the code.

Your way is better the first time someone sets the global (ie: the global is not known now), however it is confusing when one knows the global and is trying to switch it.

There is no way to toggle the global without toggling the local. and with Inverse_reading_order i want some documents RTL and some LTR.

as poire-z said whatever is decided it should be the same everywhere.

highlighting is set the inverse of your way: global true when local is false
dictionary fuzzy search and hanging punctuation are like your way.

these are the only ones that have a checkbox selector with a default that can cause this confusion all other toggles (bottom menu) have slider so you an just hold the option you want as default.

@yparitcher

This comment has been minimized.

Copy link
Contributor Author

yparitcher commented Oct 23, 2019

the clearest way would probably be to go wth three buttons cancel | disable | enable with the order of disable enable based on the current local like is currently done.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Oct 23, 2019

Pretty sure @poire-z and I already agreed on Cancel | Disable | Enable. ;-)

as poire-z said whatever is decided it should be the same everywhere.

I kind of felt that was the main point of my first reply actually. >_> :-)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 23, 2019

Yes, we all agree on 3 buttons (in a single row, or cancel on the bottom row if we go with 2 rows?).

the clearest way would probably be to go wth three buttons cancel | disable | enable with the order of disable enable based on the current local like is currently done.

Dunno if the order should be changed (it would not matter much as we would not visit them often and get no chance of getting used to the buttons order :) but in general, thing shouldn't move too much).
But if a global is set, its button might be grey/dimmed (the current global value) - so only the other is tap'able (to change the global value).
And the text should be explicite too.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Oct 23, 2019

Yeah I'd just keep the order the same. Moving them around is needlessly complex in the code and annoyingly unpredictable as a user.

@Frenzie Frenzie added the UX label Oct 23, 2019
@yparitcher

This comment has been minimized.

Copy link
Contributor Author

yparitcher commented Oct 23, 2019

how does this look?

Reader_2019-Oct-23_175619

i am using MultiConfirmBox i am not sure i can gray one out with it.

@yparitcher

This comment has been minimized.

Copy link
Contributor Author

yparitcher commented Oct 23, 2019

once we come to a final decision i will fix the other ones.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 23, 2019

i am using MultiConfirmBox i am not sure i can gray one out with it.

Looks like it does not (yet), but you can quickly make it accept some new keys:

--- a/frontend/ui/widget/multiconfirmbox.lua
+++ b/frontend/ui/widget/multiconfirmbox.lua
@@ -50,2 +50,4 @@ local MultiConfirmBox = InputContainer:new{
     cancel_callback = function() end,
+    choice1_enabled = true,
+    choice2_enabled = true,
     margin = Size.margin.default,
@@ -104,2 +106,3 @@ function MultiConfirmBox:init()
                     text = self.choice1_text,
+                    enabled = self.choice1_enabled,
                     callback = function()
@@ -111,2 +114,3 @@ function MultiConfirmBox:init()
                     text = self.choice2_text,
+                    enabled = self.choice2_enabled,
                     callback = function()

how does this look?

Good, but not super clear. (I could be tempted to hit "Disable" to get out of it :)
Dunno. Either longer text like (for this one, there is always the default of LTR) like The default for newly opened books is Left-to-Right reading order. You can set newly opened books to default to Right-To-Left reading order.
And have buttons more explicite. Either "Enable by default" and "Disable by default" (but that is long), or better: Cancel | Left-To-Right | Right-To-Left (might be short enough to stick in a single row).
(The text could read then: The default for newly opened books is Left-to-Right reading order. What should be the default or What should newly opened book reading direction be? (hope you'll find a better english wording than my cumbersome attempts :)

@yparitcher

This comment has been minimized.

Copy link
Contributor Author

yparitcher commented Oct 23, 2019

Reader_2019-Oct-23_190751
Reader_2019-Oct-23_190755

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 23, 2019

Looks good :) Even better with a newline before Would you...?

@yparitcher

This comment has been minimized.

Copy link
Contributor Author

yparitcher commented Oct 23, 2019

Reader_2019-Oct-23_193839

will start on the other ones now

yparitcher added 3 commits Oct 23, 2019
@yparitcher

This comment has been minimized.

Copy link
Contributor Author

yparitcher commented Oct 24, 2019

Reader_2019-Oct-23_195155

Reader_2019-Oct-23_200220

Reader_2019-Oct-23_200225

Reader_2019-Oct-23_201248

@yparitcher

This comment has been minimized.

Copy link
Contributor Author

yparitcher commented Oct 24, 2019

inverse reading order, enable highlighting, dictionary fuzzy search and hanging punctuation are all the same now.
3 buttons, totally independent of the local config.

@yparitcher yparitcher changed the title set global inverse_reading_order independent of current setting set global settings independent of local setting Oct 24, 2019
@Frenzie Frenzie added this to the 2019.11 milestone Oct 24, 2019
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Oct 24, 2019

I'm not overly enthused by the button disabling. Do you think it improves clarity? I like to be able to emphatically click that button, even if it is already the default.

Regardless of any disabling, I think it'd improve visual clarity and internal unity to follow the example I posted a little more closely.

@yparitcher

This comment has been minimized.

Copy link
Contributor Author

yparitcher commented Oct 24, 2019

I'm not overly enthused by the button disabling. Do you think it improves clarity? I like to be able to emphatically click that button, even if it is already the default.

it was @poire-z idea, i don't care either way. when you guys decide which way let me know and i will do so.

@yparitcher

This comment has been minimized.

Copy link
Contributor Author

yparitcher commented Oct 24, 2019

Reader_2019-Oct-24_150424
Reader_2019-Oct-24_150435
Reader_2019-Oct-24_150446

@Frenzie Frenzie merged commit aa165ce into koreader:master Oct 25, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
Frenzie added a commit that referenced this pull request Oct 25, 2019
@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 30, 2019

it was @poire-z idea, i don't care either way. when you guys decide which way let me know and i will do so.

I don't care either. I was just suggesting ideas to make these dialogs the less confusing possible - and they got less confusing.
So ok if you want to make the buttons not disabled (after all, we don't disable the current font or hyphenation in their respective lists).

Frenzie added a commit that referenced this pull request Nov 8, 2019
as per #5522 (comment) do not grey out the current default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.