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

TextWidget: small refactoring, better handle max_width #5503

Merged
merged 3 commits into from Oct 21, 2019

Conversation

@poire-z
Copy link
Contributor

poire-z commented Oct 18, 2019

Lots of code was doing some renderText calls to get the size of some text string, and truncate it to some width if needed, with or without an added ellipsis, before instantiating a TextWidget with that tweaked text string.

This PR fixes/adds some properties and methods to TextWidget so all that can be done by it. It makes the caller codes simpler, as they don't need to use RenderText directly.
(Additionally, if we ever go at using Harfbuzz for text rendering, we'll just have to update or replace textwidget.lua without the need to update any higher level code.)

RenderText: removed the space added by truncateTextByWidth after the ellipsis, as it doesn't feel needed, and break right alignment of the ellipsis with other texts.
KeyValuePage: fix some subtle size and alignment issues.
NumberPickerWidget: fix font size (provided font size was not used)

Menu.lua: I did not touch the part of the code that #5496 affects, so these 2 PRs should be mergeable in any order. If needed, I'll update what can be from #5496 to follow the spirit of this PR.
Pinging @robert00s for info, for your future widgets :)

@poire-z poire-z requested a review from robert00s Oct 18, 2019
@poire-z poire-z added this to the 2019.11 milestone Oct 18, 2019
This was referenced Oct 20, 2019
@robert00s

This comment has been minimized.

Copy link
Contributor

robert00s commented Oct 20, 2019

@poire-z
Sorry for delay. I'll try to look at it today evening or tomorrow.

Copy link
Contributor

robert00s left a comment

I'm not very familiar with textboxwidget but this PR looks really nice 👍
Please update #5496 in this way.

poire-z added 2 commits Oct 18, 2019
Lots of code was doing some renderText calls to get the
size of some text string, and truncate it to some width if
needed, with or without an added ellipsis, before instantiating
a TextWidget with that tweaked text string.

This adds some properties and methods to TextWidget so all
that can be done by it. It makes the caller codes simpler,
as they don't need to use RenderText directly.
(Additionally, if we ever go at using Harfbuzz for text
rendering, we'll just have to update or replace textwidget.lua
without the need to update any higher level code.)

KeyValuePage: fix some subtle size and alignment issues.
@poire-z poire-z force-pushed the poire-z:renderTextInTextWidget branch from 26f5d72 to f6bc3b9 Oct 21, 2019
@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Oct 21, 2019

(Just rebased to current master.)
@Frenzie : any comment on this one?

Copy link
Member

Frenzie left a comment

No real comments.

frontend/ui/widget/keyvaluepage.lua Outdated Show resolved Hide resolved
Co-Authored-By: Frans de Jonge <fransdejonge@gmail.com>
@poire-z poire-z merged commit f05e62c into koreader:master Oct 21, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@poire-z poire-z deleted the poire-z:renderTextInTextWidget branch Oct 21, 2019
Frenzie added a commit that referenced this pull request Oct 27, 2019
Set max_width like in #5503
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.