copt deduplicate: line_spacing #10768
Conversation
|
What is the order of event handling?
|
I think it does - I relied on that in the past, and even had to move self:registerModule() around to ensure that ordering. |
|
Very good, so we can use already initiated configurable settings in all the modules and avoid reading doc/global once more. This PR is a prototype. If it is okay, other settings can be deduplicated step by step, some combined, some in separate PRs to not mess up. |
| local marker_h = Screen:scaleBySize(self.ui.font.font_size * 1.1 * self.ui.font.line_space_percent * (1/100)) | ||
| local marker_h = Screen:scaleBySize(self.ui.font.font_size * 1.1 * self.configurable.line_spacing * (1/100)) |
There was a problem hiding this comment.
(So, we won't know from reading the code which module handles line spacing - but we'll know it's a "configurable" from the bottom menu. So, I guess I'm ok with that.)
| space = math.max(50, math.min(space, 200)) | ||
| self.configurable.line_spacing = space |
There was a problem hiding this comment.
Just curious: is this re-update of the self.configurable property enough for the updated value (which in practice is the same as the current one already stored by ReaderConfig) to be reflected in the bottom menu ?
I mean: if your math.max/min were ensuring other limits (ie. 110-120), would they reflect in the bottom widget (progress bar updated, later launch of NumberPicker from ... showing that value)?
There was a problem hiding this comment.
Yes, the clamped value is applied, saved and the bottom menu progress bar and spinwidget show it.
There was a problem hiding this comment.
Directly while the bottom menu is still opened ? Or do you need to close & reopen it?
There was a problem hiding this comment.
On "Apply" the spinwidget is not updated (shows still unclamped value), the notification shows the clamped value applied, the bottom menu at this moment is hidden, then we close the spinwidget and the reappearing bottom menu shows actual (clamped) value.
There was a problem hiding this comment.
What when you don't use the spinwidget, but just tap on the left or right most progress bar squares?
There was a problem hiding this comment.
Bottom menu is updated after redrawing the page (with the new clamped value), and the progress bar shows it correctly.
There was a problem hiding this comment.
Thanks. So, all that sounds rather good to me :)
Dunno if it will be as clean and simple from some of the other bottom settngs (ie; view_mode page/scroll).
There was a problem hiding this comment.
That's why "step by step".
There was a problem hiding this comment.
Yes :) But good to try a second step with a more complex case, to be sure this dance will look good and end well :)
|
Can this be merged? To continue over. |
|
Have you tried (on your side) this idea with another more complex setting? |
|
I've made for font size, weight, hinting, kerning. |
|
Looks good to me. |
|
Well, what would make more sense would still be that each module manage fully its settings - and that the bottom menu would not manage its own state. And that a unique setting could be made available in the top or in the bottom menu without much differences. This PR make the bottom menu the main state and setting handler. It means that if tomorrow we want to move a setting from the bottom menu to a toggle in the top menu, we'd have to rewrite a bunch of stuff (and live with a setting named copt_something that is not managed by the bottom menu - or do a onetime migration).
Given the horror and complexity the bottom menu stuff is, I have no idea how feasible this is :/ |
|
Yup, @poire-z's perfectly summarized my thoughts on the subject. ConfigDialog is a nightmare, but it's a nightmare that sorta works ;p. |
|
Another question, as I sometimes do that (for checking changes/regressions): what will happen if I open a doc with the future version of KOReader with this PR - and then go at launching an older version of KOReader and open that same doc ? |
|
The doc will take the default (global) values of the settings that have been deduplicated. |
Main discussion in #10763.
This change is