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

Footer - add option chapter markers width #5708

Merged
merged 5 commits into from Dec 30, 2019
Merged

Conversation

@robert00s
Copy link
Contributor

robert00s commented Dec 28, 2019

Move DMINIBAR_TOC_MARKER_WIDTH from Advanced settings to Status bar -> Progress bar -> Style

  • Thin (DMINIBAR_TOC_MARKER_WIDTH =1)
  • Normal (DMINIBAR_TOC_MARKER_WIDTH =2)
  • Thick (DMINIBAR_TOC_MARKER_WIDTH =3)

This change is Reviewable

robert00s added 2 commits Dec 28, 2019
@Frenzie Frenzie added this to the 2019.12 milestone Dec 28, 2019
@Frenzie Frenzie added the UX label Dec 28, 2019
Copy link
Member

Frenzie left a comment

Seems fine to me, but only taking a quick look atm

if self.settings.toc_markers_width == 1 then
markers_width_text = _("thin")
elseif self.settings.toc_markers_width == 2 then
markers_width_text = _("normal")

This comment has been minimized.

Copy link
@Frenzie

Frenzie Dec 28, 2019

Member

I'd probably say either medium or default.

elseif self.settings.toc_markers_width == 2 then
markers_width_text = _("normal")
end
return T(_("Chapter markers width (%1)"), markers_width_text)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Dec 28, 2019

Member
Suggested change
return T(_("Chapter markers width (%1)"), markers_width_text)
return T(_("Chapter marker width (%1)"), markers_width_text)
end,
},
{
text = _("Normal"),

This comment has been minimized.

Copy link
@Frenzie

Frenzie Dec 28, 2019

Member

medium/default?

checked_func = function()
return self.settings.toc_markers_width == 1
end,
callback = function()
self.settings.toc_markers_width = Screen:scaleBySize(1)
Comment on lines 1187 to 1191

This comment has been minimized.

Copy link
@poire-z

poire-z Dec 28, 2019

Contributor

Looks like this might work on an emulator where scaleBySize does x1 - but unless I miss something, it won't be checked (or worse) when scaleBySize does x1.7 or x2.
So, may be just save the unscale value - and do the scaleBySize on lines 1330 and/or 338.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Dec 28, 2019

(An other option, for later :), might be to have the marker width larger for top level TOC items, and thinner for lower levels - but I remember there's some code that only take items from a single level when showing marks, #5500 (comment).)

@robert00s

This comment has been minimized.

Copy link
Contributor Author

robert00s commented Dec 29, 2019

How to ignore this?

Checking for unscaled sizes

Warning: it looks like you might be using unscaled sizes.
It is almost always preferable to defer to one of the predefined sizes in ui.size in the following files:
frontend/apps/reader/modules/readerfooter.lua:1191:                                    self.settings.toc_markers_width = 1
frontend/apps/reader/modules/readerfooter.lua:1202:                                    self.settings.toc_markers_width = 2
frontend/apps/reader/modules/readerfooter.lua:1213:                                    self.settings.toc_markers_width = 3

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Dec 29, 2019

You could either add it to the list of exceptions (that grep -Ev '(default_option_height|default_option_padding)' part) or write the variable name in such a way that it doesn't trigger. Perhaps we could add a generic unscaled_width/height override.

unscaled_size_check=$(grep -nr --include=*.lua --exclude=koptoptions.lua --exclude-dir=base --exclude-dir=luajit-rocks --exclude-dir=install --exclude-dir=keyboardlayouts --exclude-dir=*arm* "\\(padding\\|margin\\|bordersize\\|width\\|height\\|radius\\|linesize\\) = [0-9]\\{1,2\\}" | grep -v '= 0' | grep -v '= [0-9]/[0-9]' | grep -Ev '(default_option_height|default_option_padding)' | grep -v scaleBySize || true)

The point of the script is merely to force you to think about it, and 9 times out of 10 it'll remind you not to do something stupid. The other time you have to amend the script or work around it.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Dec 29, 2019

Or just add to that line a generic grep -v 'unscaled_size_check: ignore" , so we can just use (after we have thought about it :):

self.settings.toc_markers_width = 1 -- unscaled_size_check: ignore
@robert00s

This comment has been minimized.

Copy link
Contributor Author

robert00s commented Dec 29, 2019

Thanks for tip :)

ci
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Dec 29, 2019

True, it's probably clearer that way. 👍

@Frenzie Frenzie merged commit dd0c6c0 into koreader:master Dec 30, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@robert00s robert00s deleted the robert00s:marker_width branch Jan 2, 2020
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.