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

Button related tweaks, add menu to Book map and Page browser #10230

Merged
merged 10 commits into from
Mar 23, 2023

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Mar 20, 2023

Easier to review commit by commit.
Fixes some issues noticed at #9526 (comment).
Implement ideas discussed at #10056 (comment)

Button: handle 'width' as the final outer width

All our widgets are considering their provided 'width' as the outer width, except Button which considered it as some 'inner width', to which padding/border/margin were added. Let's have them all consistent.
Some other widgets using Button had tweaks to account for that odd behaviour: fix and simplify them.
Also fix Button layout when text is left aligned.

FrontLightWidget: cleanup buttons layout

Properly compute Button and separator widths, instead of using magic numbers (which lost their magic over the years :)
Ensure buttons and progress widgets are properly aligned on the sides.
Move the optional Warmth "Configure" button in the middle of Warmth Min and Max.
Fix keyboard navigation layout, which was not working on devices with Warmth.

Button and layout should look now better aligned (no idea why it goes with 2 different progress widget, but I don't care).
image

Button: fix enableDisable when multilines

Also add support for a enabled_func() property, and ensure its enable/disable state on each repaint.

ButtonTable: allow buttons to request a fixed width

It is expected some button(s) in a row do not specify such a width, so they get distributed the remaining space from the ButtonTable specified width.
This is to allow building these -/+ buttons:
image

ButtonDialog/ButtonDialogTitle: consistent 'width' handling

Make these 2 widget behave similarly, and don't rely on ButtonTable default width for their own default width.
ButtonDialogTitle: also properly size its title.

ButtonTable: allow shrinking uneeded width

If buttons and their text would fit in a smaller width, reduce the whole ButtonTable width.
May be used for properly sized context menus with ButtonDialog, with the width required depending on the translations for a language.

MovableContainer: add support for anchoring initial position

When passing as 'anchor' a Geom object (ie. a widget dimen or a ges.pos), or a function returning such an object, a MovableContainer will be initially positionned near this point/widget, instead of being centered on the screen.
Allow that for ButtonDialog and ButtonDialogTitle, so we can make them behave as context menus (ie. for a titlebar top left/right icons).

Book map, Page browser: add top left menu

Split original very long help text (which was very slow to display) into 2 parts: About... and Available gestures.
Also add -/+ buttons to change things (which can already and more practically be done with swipes along the edges) to give a bit of meat to these menus.
I also modified a bit the wording (ie. my pompous "Under xxx can be found yyy...").

image

image

image
(in there, I went with adding a blank line between each sentence, makes it easier to read to me, is that ok?).

image

image

image

Book style tweak: add button with CSS suggestions

Mostly non-standard CSS declarations (so, possibly unknown to users) that can be useful for solving certain issues.
Long-press on them show a little help_text.

image

I think one or two of these should go, because it gets a bit too tall and get shifted and in the way.
I added the text-indent, font-size and margins one early to have something a bit consistent, and later added more -cr-hint. Which of these brings less knowledge and can go ?

bump crengine: fix parsing of multibytes encodings

Includes koreader/crengine#515 :


This change is Reviewable

@poire-z poire-z added this to the 2023.04 milestone Mar 20, 2023
@poire-z poire-z added the UX label Mar 20, 2023
@poire-z
Copy link
Contributor Author

poire-z commented Mar 20, 2023

For info, I experimented with having and using:

--- a/frontend/apps/reader/modules/readerhighlight.lua
+++ b/frontend/apps/reader/modules/readerhighlight.lua
@@ -969,8 +969,10 @@ function ReaderHighlight:onShowHighlightMenu(page, index)
     local highlight_buttons = {{}}

     local columns = 2
+columns = 1
     for idx, fn_button in ffiUtil.orderedPairs(self._highlight_buttons) do
         local button = fn_button(self, page, index)
+button.align = "left"
         if not button.show_in_highlight_dialog_func or button.show_in_highlight_dialog_func() then
             if #highlight_buttons[#highlight_buttons] >= columns then
                 table.insert(highlight_buttons, {})
@@ -982,6 +984,8 @@ function ReaderHighlight:onShowHighlightMenu(page, index)

     self.highlight_dialog = ButtonDialog:new{
         buttons = highlight_buttons,
+anchor = self.holdpan_pos or self.hold_pos,
+shrink_uneeded_width = true,
         tap_close_callback = function() self:handleEvent(Event:new("Tap")) end,
     }
     -- NOTE: Disable merging for this update,

getting a more computer-like context menu.
It would actually look a lot better on a computer screen in portrait:

zzz

and it wouldn't look too bad on a device - but it's a lot less practical than the big centered 2-columns menu we have - you'd get a bit surprised each time it displays, not at the same place, and I missed the 2 columns which helps getting to the needed button, easier than with this tall pile of buttons.

@poire-z
Copy link
Contributor Author

poire-z commented Mar 20, 2023

I also experimented with having this on long-press on Page browser's thumbnails:

--- a/frontend/ui/widget/pagebrowserwidget.lua
+++ b/frontend/ui/widget/pagebrowserwidget.lua
@@ -1000,10 +1141,50 @@ function PageBrowserWidget:onHold(arg, ges)
                 -- handle)
                 local thumb_frame = self.grid[idx][1][1]
                 if ges.pos:intersectWith(thumb_frame.dimen) then
+--[[
                     self.ui.bookmark:toggleBookmark(page)
                     -- Update our cached bookmarks info and ensure the bottom ribbon is redrawn
                     self.bookmarked_pages = self.ui.bookmark:getBookmarkedPages()
                     self:updateLayout()
+]]--
+                    local button_dialog
+                    local buttons = {
+                        {{
+                            text = _("Toggle page bookmark"),
+                            align = "left",
+                            callback = function()
+                                UIManager:close(button_dialog)
+                                self.ui.bookmark:toggleBookmark(page)
+                                -- Update our cached bookmarks info and ensure the bottom ribbon is redrawn
+                                self.bookmarked_pages = self.ui.bookmark:getBookmarkedPages()
+                                self:updateLayout()
+                            end,
+                        }},
+                        {{
+                            text = _("Start homemade TOC chapter here"),
+                            align = "left",
+                            callback = function()
+                            end,
+                        }},
+                        {{
+                            text = _("Start/end non-linear flow here"),
+                            align = "left",
+                            callback = function()
+                            end,
+                        }},
+                    }
+                    button_dialog = ButtonDialog:new{
+                        -- width = math.floor(Screen:getWidth() / 3),
+                        width = math.floor(Screen:getWidth() *9/10),
+                        shrink_uneeded_width = true,
+                        buttons = buttons,
+                        anchor = ges.pos,
+                    }
+                    UIManager:show(button_dialog)
+
                     return true
                 end
             end

pagebrowserthumbnailcontextmenu

which feels a bit more sensical (or just new, no habit yet gotten :).
But as I don't yet want to implement these 2 other actions, we only have one - so no need for that context menu :)

@poire-z poire-z mentioned this pull request Mar 20, 2023
@poire-z poire-z force-pushed the button_width_and_co branch from 30ac276 to 9b22a61 Compare March 20, 2023 19:27
@hius07
Copy link
Member

hius07 commented Mar 21, 2023

Missed double n in shrink_uneeded_width?

@poire-z poire-z force-pushed the button_width_and_co branch from 9b22a61 to 992fbbe Compare March 21, 2023 12:29
@weijiuqiao
Copy link
Contributor

Sorry I haven't really gotten around to test in VocabBuilder plugin. Changed to an ARM mac recently and am anticipating problems getting the emulator to spin (hope not).

@poire-z poire-z force-pushed the button_width_and_co branch from 992fbbe to 0543321 Compare March 22, 2023 12:58
@poire-z
Copy link
Contributor Author

poire-z commented Mar 22, 2023

Updated last commit:

  • removed "margin: TRBL", reorder by alphabetical order
  • changed the Insert... button to (untranslatable) CSS+... (as it's CSS, and mostly crengine extended CSS, so ++, that will be inserted) - but I'm open to rewording if that's too cryptic.

image

Any feedback about the wordings in the last 2 commits ?

@@ -771,6 +771,16 @@ local BOOK_TWEAK_INPUT_HINT = T([[

%2]], _("You can add CSS snippets which will be applied only to this book."), BOOK_TWEAK_SAMPLE_CSS)

local CSS_SUGGESTIONS = {
{ "-cr-hint: footnote-inpage;", _("When set on a block element containing the target id of a href, this block element will be shown as an in-page footnote.")},
{ "-cr-hint: non-linear-combining;", _("Can be set on some specific DocFragment (ie. DocFragment[id*=16]) to ignore them in the linear pages flow.")},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either docfragments/them or docfragment/it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you write it after I tell you DocFragment is a element tag name (<DocFragment>, like <b>), and that I'd like to keep the plural "some specific b(s).. them" ?
"some specific bs (ie b[id*=16])" ?
Feels strange to add a s to that tag name - but it might be safe as there is an example in the (ie. b[id*=16]) just behind it.

{ "-cr-hint: non-linear-combining;", _("Can be set on some specific DocFragment (ie. DocFragment[id*=16]) to ignore them in the linear pages flow.")},
{ "-cr-hint: toc-level1;", _("When set on an element, its text can be used to build the alternative table of contents.")},
{ "display: run-in !important,", _("When set on a block element, this element content will be inlined with the next block element.")},
{ "font-size: 1rem !important;", _("1rem will use your default font size")},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your or just the?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought "your" is clearer (the one you have set in koreader), "the" is ambiguous (the book one ? the default for the element ?...)

But, even "default" is ambiguous :/ "the font size you have set in the bottom menu" is long... :\

@Frenzie
Copy link
Member

Frenzie commented Mar 22, 2023

Pinging @weijiuqiao; this should be okay for Latin alphabet languages anyway.

(untranslatable) CSS+...

},
{
{
text = _("Page slots width"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Page slot widths? I'm not completely sure what this is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is mentionned (was and still is, didn't change its wording) in the help text:
image

Its for going from less or more wide page slots:
image
image
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then just make it page slot width. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK... always confused with these kind of English constructs....
Are you ok with my "Thunbnail rows" and "Thumbnail columns" with Thumbnail singular ? (meaning, number of thumbnail rows, how many rows of thumbnails...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's the opposite that'd be weird. ^_^ "Page slot widths" would be fine too but I don't think that's intended here?

UIManager:show(InfoMessage:new{
text = _([[
Book map displays an overview of the book content.

If statistics are enabled, black bars are shown for already read pages (gray for pages read in the current reading session). Their heights vary with the time spent reading the page.
Chapters are shown above their pages.
Under the pages can be found some indicators:
Chapters are shown above the pages they span.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

encompass?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

span (v)
verb (used with object), spanned, span·ning.
to measure by the hand with the thumb and little finger extended.
to encircle with the hand or hands, as the waist.
to extend over or across (a section of land, a river, etc.).
to provide with something that extends over: to span a river with a bridge.
to extend or reach over (space or time): a memory that spans 90 years.
Mathematics. to function (in a subspace of a vector space) as a span.
Archery. to bend (the bow) in preparation for shooting.

looked ok to me :)
But ok with encompass.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd take span more literally (as in visually span), although on the book map maybe that's exactly what you meant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what it means I guess, chapters (titles) are shown above the pages (slots) they span/encompass:
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they do span it quite literally. :-)

A chapter does span multiple pages of course, but somehow it just seems a bit weird to me to put it that way. I think perhaps because it's the opposite that's remarkable, a chapter that doesn't span multiple pages.

Another possible word, maybe contain?

@weijiuqiao
Copy link
Contributor

Pinging @weijiuqiao; this should be okay for Latin alphabet languages anyway.

(untranslatable) CSS+...

I'm sure anyone who will use it would understand what CSS is. That being said, how about Insert CSS? It seems easier to understand (if I'm not misunderstanding the intension of the button).

@poire-z
Copy link
Contributor Author

poire-z commented Mar 22, 2023

That being said, how about Insert CSS? It seems easier to understand (if I'm not misunderstanding the intension of the button).

You are not misunderstanding. It's exactly that, but if you just say "Insert CSS", one may thing it has to go thru this to insert any CSS - and/or it is limited to the proposed 7 CSS thingies. (CSS+ feels like it expresses both insertion, and non-obvious CSS, but that may just be me :)
It also feels like it needs these ... as it's not a direct action.
Could also become congested and use a smaller font if we make it longer:
image

(That screenshots shows that we don't (past and present) use any padding_h in ButtonTable when the text is centered - with using the same padding_h I added for left aligment, we could get:
image )

@weijiuqiao
Copy link
Contributor

Then I'm a little biased towards +CSS... :) Maybe because I'm more used to LTR and VO languages.

@Frenzie
Copy link
Member

Frenzie commented Mar 22, 2023

  • CSS could work but then it should be more of an iconic + I think, like ➕ for example (heavy plus sign).

The bottom ribbon displays an extract of the book map around the shown pages: see the book map help for details.
The bottom ribbon displays an extract of the book map around the pages displayed:

If statistics are enabled, black bars are shown for already read pages (gray for pages read in the current reading session). Their heights vary with the time spent reading the page.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

depending on

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

full sentence please :)
(I didn't like "vary", so open for a full better sounding sentence.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think vary sounds alright when combined with depending on, is what I intended to say. :-)

Their heights vary depending on the time spent reading the page.

@poire-z
Copy link
Contributor Author

poire-z commented Mar 22, 2023

A bit heavy, but OK :)

(I would read that semantically as : "CSS ++/extended/more powerful ...")
image

(I would read that semantically as : "More (that you didn't even know about) CSS ...")
image

Then I'm a little biased towards +CSS... :) Maybe because I'm more used to LTR and VO languages.

(I'm also LTR minded, but +CSS looks odd :) but ok given my reading above)

So, which one ? With ellipsis... or without?

@Frenzie
Copy link
Member

Frenzie commented Mar 22, 2023

That was just a sample (and on my phone that wasn't… yellow). I was really just trying to say that we can look beyond + for a few minutes and always fall back to + if nothing seems appropriate.

image

Anyway, the + from the following screenshot was fine.
image

@mergen3107
Copy link
Contributor

How about CSS and little drop down or up arrow? Like on a drop down list? Because that is what it looks like when hit the CSS button.

@poire-z
Copy link
Contributor Author

poire-z commented Mar 22, 2023

How about CSS and little drop down or up arrow? Like on a drop down list? Because that is what it looks like when hit the CSS button.

Nice and obvious idea ! Better than the ellipsis to explain that fact :)
(And I'm using this character in the help text to mimic the top left svg icon...).

With a space:
image

Without any spacing:
image

With a + (now it feels a bit odd, might no longer be needed):
image

With a superscript +:
image

It now feels the CSS ≡ doesn't need anything else - it invites to just be hit without any direct effect other than showing a list, and the user sees that list, and understand (or not) what it's all about from its content.

@Frenzie
Copy link
Member

Frenzie commented Mar 22, 2023

Agreed, clear by itself.

@mergen3107
Copy link
Contributor

Yes, by itself looks very self-explanatory

@poire-z poire-z force-pushed the button_width_and_co branch from 0543321 to b3d0643 Compare March 23, 2023 07:55
poire-z added 2 commits March 23, 2023 18:37
All our widgets are considering their provided 'width'
as the outer width, except Button which considered it
as some 'inner width', to which padding/border/margin
were added. Let's have them all consistent.
Some other widgets using Button had tweaks to account
for that odd behaviour: fix and simplify them.
Also fix Button layout when text is left aligned.
Properly compute Button and separator widths,
instead of using magic numbers (which lost their
magic over the years :)
Ensure buttons and progress widgets are properly
aligned on the sides.
Move the optional Warmth "Configure" button in the
middle of Warmth Min and Max.
Fix keyboard navigation layout, which was not working
on devices with Warmth.
poire-z added 8 commits March 23, 2023 18:37
Also add support for a enabled_func() property, and
ensure its enable/disable state on each repaint.
It is expected some button(s) in a row do not specify
such a width, so they get distributed the remaining
space from the ButtonTable specified width.
Make these 2 widget behave similarly, and don't rely on
ButtonTable default width for their own default width.
ButtonDialogTitle: also properly size its title.
If buttons and their text would fit in a smaller width,
reduce the whole ButtonTable width.
May be used for properly sized context menus with
ButtonDialog, with the width required depending on
the translations for a language.
When passing as 'anchor' a Geom object (ie. a widget dimen
or a ges.pos), or a function returning such an object,
a MovableContainer will be initially positionned near this
point/widget, instead of being centered on the screen.
Allow that for ButtonDialog and ButtonDialogTitle, so we
can make them behave as context menus (ie. for a titlebar
top left/right icons).
Split original very long help text (which was very
slow to display) into 2 parts: About... and Available
gestures.
Also add -/+ buttons to change things (which can already
and more practically be done with swipes along the edges)
to give a bit of meat to these menus.
Mostly non-standard CSS declarations (so, possibly unknown
to users) that can be useful for solving certain issues.
Includes:
- LVTextFileBase: fix parsing of multibytes encodings
@poire-z poire-z force-pushed the button_width_and_co branch from b3d0643 to 83166f4 Compare March 23, 2023 17:38
@poire-z poire-z merged commit b3fe90c into koreader:master Mar 23, 2023
@poire-z poire-z deleted the button_width_and_co branch March 23, 2023 19:28
@poire-z
Copy link
Contributor Author

poire-z commented Mar 23, 2023

Just mentionning some bits from my notes while checking which widgets needed some update with this button width handling change:

  • In ReaderToC, the expand/collapse icons may change width/position (they would be now a tad smaller if I remember right), and it's probably hardly noticable - I didn't try to make it look as before
  • in KeyValuePage and SortWidget, the bottom icons should have the same visual size, but their widget may be a tad larger - no visual change I guess because the large width they occupy has enough room so that these increased individual width find some place.

TranHHoang added a commit to TranHHoang/koreader that referenced this pull request May 26, 2023
KOReader 2023.04 "Solar Panel"

![koreader-2023-04-fs8](https://user-images.githubusercontent.com/202757/234975122-d7739c71-74aa-4cb0-a086-72a4bba70f52.png)

It's been another busy month squashing many bugs. Our Mac users will be happy to hear that I told macOS we've supported HiDPI since long before anyone came up with such terminology (koreader#10341), and that the program can now natively build on M1 devices (koreader#10291).

Solar panel credit: <https://openclipart.org/detail/294030/solar-energy> by gnokii

We'd like to thank all contributors for their efforts. Some highlights since the previous release include:

* Readerzooming: fix use of default settings (koreader#10205) @hius07
* ButtonDialog/ButtonDialogTitle: consistent 'width' handling (koreader#10230) @poire-z
* MovableContainer: add support for anchoring initial position (koreader#10230) @poire-z
* Book map, Page browser: add top left menu (koreader#10230) @poire-z
* Book style tweak: add button with CSS suggestions (koreader#10230) @poire-z
* crengine: fix parsing of multibytes encodings (koreader#10230) @poire-z
* readerstyletweak: update profiles on unregistering in dispatcher (koreader#10247) @hius07
* Filesearcher: add search in book metadata (koreader#10198) @hius07
* Fix: Updated legacy directory, which crashed the program (koreader#10260) @Mochitto
* ReaderLink: allow a forward location stack (koreader#10228) @yparitcher
* BookInfo: add page information (koreader#10255) @hius07
* Center pdf manual zoom mode (koreader#10246) @nairyosangha
* File browser: add Folder Menu (koreader#10275) @hius07
* Calendar view: add options to change start time of days (koreader#10254) @weijiuqiao
* filechooser: fix crash on "unreadable content" (koreader#10283) @hius07
* Sync book statistics: add to dispatcher (koreader#10285) @ptrm
* [plugin] Exporter: use util.getSafeFilename() to remove illegal characters from output filename (koreader#10282) @Mochitto
* readerbookmark: fix writing pdf annotation (koreader#10287) @hius07
* Folder Menu: sign for Home folder (koreader#10288) @hius07
* ListMenu: show mark for books with highlights (koreader#10276) @hius07
* Calendar view's day view: thicker separator at 00:00 (koreader#10289) @poire-z
* PageBrowser: tweak scrolling behaviour at book start/end (koreader#10289) @poire-z
* Make `kodev check` feature complete (koreader#8682) @yparitcher
* macOS: support for M1 building (koreader#10291) @ptrm
* Reader: do not apply font size and spacing out of range (koreader#10295, koreader#10307) @hius07
* File browser: show Folder Menu on long-press on Home icon (koreader#10298) @hius07
* SSH.koplugin: fix cant stop SSH server bug when pid file's stale (koreader#10300) @weijiuqiao
* PM: Optimize task queue handling around standby (koreader#10203) @zwim
* statistic.koplugin: fix today's timeline showing next day when within custom offset (koreader#10299) @weijiuqiao
* ReaderThumbnails: update cached page thumbnail on bookmark note change (koreader#10303) @hius07
* SDL: add multitouch support (koreader#10334) @Frenzie
* SDL: add HiDPI support (koreader#10341) @Frenzie
* BookInfo: fix crash on show cover (koreader#10315) @hius07
* Deal with table.pack corner-cases properly (koreader#10350) @NiLuJe
* Android: add Tagus Gea support (<koreader/android-luajit-launcher#412>) @Alfedi

[Full changelog](koreader/koreader@v2023.03...v2023.04) — [closed milestone issues](https://github.com/koreader/koreader/milestone/63?closed=1)

---

Installation instructions: [Android](https://github.com/koreader/koreader/wiki/Installation-on-Android-devices) • [Cervantes](https://github.com/koreader/koreader/wiki/Installation-on-BQ-devices) • [ChromeOS](https://github.com/koreader/koreader/wiki/Installation-on-Chromebook-devices) • [Kindle](https://github.com/koreader/koreader/wiki/Installation-on-Kindle-devices) • [Kobo](https://github.com/koreader/koreader/wiki/Installation-on-Kobo-devices) • [PocketBook](https://github.com/koreader/koreader/wiki/Installation-on-PocketBook-devices) • [ReMarkable](https://github.com/koreader/koreader/wiki/Installation-on-ReMarkable) • [Desktop Linux](https://github.com/koreader/koreader/wiki/Installation-on-desktop-linux) • [MacOS](https://github.com/koreader/koreader/wiki/Installation-on-MacOS)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Garbled characters when reading gb2312 text file. when open gb2312 text file,there are some garble words
5 participants