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

[UX] creoptions: add more margin values #4691

Merged
merged 2 commits into from Mar 1, 2019

Conversation

Projects
None yet
3 participants
@Frenzie
Copy link
Member

Frenzie commented Mar 1, 2019

Fixes #4684.

screenshot_2019-03-01_12-55-22

@Frenzie Frenzie added the UX label Mar 1, 2019

@Frenzie Frenzie added this to the 2019.03 milestone Mar 1, 2019

@Frenzie Frenzie requested a review from poire-z Mar 1, 2019

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Mar 1, 2019

👍

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 1, 2019

Looks fine.
Glad you didn't change the 3 first values (so, no shift in settings - no book re-rendering or visual changes for users) and only added new values.

(It will just be a bit strange for people who did customized the 3 first ones in default.persistent.lua - probably they increased them, possibly beyond the 4th and 5th).

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 1, 2019

Nothing to do with this PR, just brainstorming (even for a possible personal hack):
On my GloHD, a refresh on a previously solid black area makes the underlying test grayer/thiner instead of black.
So, with these black progress bars like contrast and this new one, I usually follow quiting the bottom menu with a small diagonal swipe to full refresh.
I don't think it's worth doing a full refresh when quiting a bottom config dialog that would include these black bars - might not be needed on other devices.
Just wondering if there's a visual idea for such progress bar without the big solid blacks - while still as readable as to what is selected. I can't think of any :|

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Mar 1, 2019

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Mar 1, 2019

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Mar 1, 2019

I don't think it'd suffer in usability from using the same gray as the toggle switches, if that's any better for ghosting without a flash.

PS I'll fix the unit tests later.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 1, 2019

Well, solid gray areas auto-flash-ui themselves on my GloHD :) and I really don't like that either (I've converted some of our gray lines to black in my own patch - separators in the menu were flashing and that did bother me).
But ok, I don't mind that much the gray toggles, so I'll try that progress bar dressed in gray.

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Mar 1, 2019

solid gray areas auto-flash-ui themselves

I might be overlooking something (especially since I haven't looked at the code and am therefore unaware of its comments) but I'd say either so should black or neither should gray, depending on which behavior is best?

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 1, 2019

No worry with our code. It's the (GloHD at least) hardware that does this auto-flash with grey (it's not a single transition: it goes from white to black, and they from black to grey - or something like that - no matter what we use in our code).

},
name_text_hold_callback = optionsutil.showValuesMargins,

This comment has been minimized.

@Frenzie

Frenzie Mar 1, 2019

Author Member

Whoops, this deletion is a mistake. :-)

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Mar 1, 2019

Made it slightly prettier:

screenshot_2019-03-01_15-07-30

Frenzie added some commits Mar 1, 2019

[spec] Fix defaults_spec
Updated for #4691

Also the assert.is_same() argument order was wrong.
The first argument is expected, the second the real-life result.
Otherwise the error message in case of failure is misleading.

@Frenzie Frenzie force-pushed the Frenzie:cre-margins branch from 93f307a to ab25c63 Mar 1, 2019

@Frenzie Frenzie merged commit 89fe3e3 into koreader:master Mar 1, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Frenzie Frenzie deleted the Frenzie:cre-margins branch Mar 1, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 1, 2019

I don't think it'd suffer in usability from using the same gray as the toggle switches, if that's any better for ghosting without a flash.

It's actually far better in grey on my GloHD (because of the HW auto-flash, which leaves no ghosting at all):

image

image

A bit too monotonous though :)

In case one wantd to try it on some other devices:

--- a/frontend/ui/widget/buttonprogresswidget.lua
+++ b/frontend/ui/widget/buttonprogresswidget.lua
@@ -43,13 +43,15 @@ function ButtonProgressWidget:update()
     local button_margin = Size.margin.tiny
     local button_padding = Size.padding.button
     local button_bordersize = Size.border.button
-    local preselect
+    local preselect, background
     local button_width = math.floor(self.width / self.num_buttons) - 2*button_padding - 2*button_margin - 2*button_bordersize
     for i = 1, self.num_buttons do
         if self.position >= i then
             preselect = true
+            background = Blitbuffer.COLOR_GREY
         else
             preselect = false
+            background = Blitbuffer.COLOR_WHITE
         end
         local button = Button:new{
             text = "",
@@ -60,6 +62,7 @@ function ButtonProgressWidget:update()
             enabled = true,
             width = button_width,
             preselect = preselect,
+            background = background,
             text_font_face = self.font_face,
             text_font_size = self.font_size,
             callback = function()

Although I vaguely recall having removed the flashing from the bottom menu to avoid bad interactions with the top menu ;).

Indeed, no flash on the bottom menu.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 2, 2019

Holding on margin title (to have help text) now crashes with:

./luajit: frontend/ui/data/optionsutil.lua:88: attempt to get length of field 'toggle' (a nil value)
stack traceback:
        frontend/ui/data/optionsutil.lua:88: in function 'name_text_hold_callback'
        frontend/ui/widget/configdialog.lua:277: in function 'hold_callback'
        frontend/ui/widget/button.lua:241: in function 'handleEvent'
        frontend/ui/widget/container/inputcontainer.lua:255: in function 'handleEvent'

(No crash when holding on the Contrast title)

Frenzie added a commit to Frenzie/koreader that referenced this pull request Mar 2, 2019

Frenzie added a commit that referenced this pull request Mar 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.