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

New widget DoubleSpinWidget based on HyphenationLimits #5679

Merged
merged 3 commits into from Dec 11, 2019

Conversation

@robert00s
Copy link
Contributor

robert00s commented Dec 10, 2019

More generic HyphenationLimits widget is now known as DoubleSpinWidget. TextBoxWidget was changed to TextWidget to prevent multiline in title above NumberPickerWidgets.

This is first step to create more_option ⋮ for Word Spacing in ConfigDialog.
See: #5655 (comment)


This change is Reviewable

@robert00s robert00s requested a review from poire-z Dec 10, 2019
@robert00s robert00s added the UX label Dec 10, 2019
Copy link
Contributor

poire-z left a comment

Good idea to make that hyphenationlimits widget more generic!

-- Min (2) and max (10) values are enforced by crengine
left_min = 1,
left_max = 10,
left_value = 2,
Comment on lines 31 to 34

This comment has been minimized.

Copy link
@poire-z

poire-z Dec 10, 2019

Contributor

May be put back these values (and the comment) in readerhyphenation.lua.
And here, set defaults similar to the ones of (single)spinwidget, for coherency (even if no other widget will use these defaults).

right_min = 1,
right_max = 10,
right_value = 2,
right_default = nil,
right_text = _("Right"),
button_default_text = _("Use language defaults"),

This comment has been minimized.

Copy link
@poire-z

poire-z Dec 10, 2019

Contributor

This text belongs to readerhyphenation, it's really not a good default for DoubleSpinWidget :)
Either a more generic _("Use defaults"), or make that additional button optional if that value is nil (and use nil as the default).

@robert00s

This comment has been minimized.

Copy link
Contributor Author

robert00s commented Dec 11, 2019

@poire-z
Thanks for review :)

@robert00s robert00s added this to the 2019.12 milestone Dec 11, 2019
Copy link
Contributor

poire-z left a comment

Lgtm.

@@ -39,13 +39,20 @@ function ReaderHyphenation:init()
local alg_left_hyphen_min = hyph_settings.left_hyphen_min
local alg_right_hyphen_min = hyph_settings.right_hyphen_min
local hyph_limits_widget = DoubleSpinWidget:new{
-- Min (2) and max (10) values are enforced by crengine

This comment has been minimized.

Copy link
@poire-z

poire-z Dec 11, 2019

Contributor

OK, I must have written that false comment before some change I made to crengine :)

// min value supported by algorithms is 1 (max is arbitrary 10)
// value enforced by algorithm previously was 2, so it's the default
#define HYPH_DEFAULT_HYPHEN_MIN 2
#define HYPH_MIN_HYPHEN_MIN 1
#define HYPH_MAX_HYPHEN_MIN 10

Can you please correct it, and just change Min (2) to Min (1), so it's coherent with the left_min = 1, right_min = 1,?

@poire-z poire-z merged commit e26ad2b into koreader:master Dec 11, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Dec 11, 2019

(@robert00s : please see #5678 (comment) if you missed it)

@robert00s robert00s deleted the robert00s:doublespinwidget branch Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.