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

Links menu and handling tweaks #4867

Merged
merged 1 commit into from Apr 2, 2019

Conversation

Projects
None yet
3 participants
@poire-z
Copy link
Contributor

commented Apr 2, 2019

Follow up to #4863, see discussion there (so, I removed the External link action menu, and as a compensation, I allowed external links on swipe :).

  • Removed "Swipe to follow first link on page" menu item and handling code, as it feels not really as practical as "Swipe to follow nearest link".
  • Removed recently added "External link action", as we can just always present a popup with the url and the available actions.
  • Generic handling of these actions in onGoToExternalLink(), so they are proposed on the Wikipedia lookup popup too.
  • Allow external link on PDF documents (previously, only internal links were handled).
  • Have "Ignore external links on tap" available on all document types.
  • Added "Ignore external links on swipe" (default to true, the current behaviour).
  • Added multiswipe gesture "Follow nearest internal link" (the existing "Follow nearest link" now follows the nearest external or internal link)
  • ButtonDialogTitle: added an option to look a bit more alike ConfirmBoxes.
  • Footnote popups: fix link unhighlight when tap on external link.

I'm not really fond of the switch for Wikipedia popup from the ConfirmBox & MultiConfirmBox to this ButtonDialogTitle, but I guess I'll get used to it (the removal of the Info icon make more room for the article title, so I guess it's good).

image

For all Wikipedia or external links, the popup will be the same, with just added buttons depending on what's possible/available:

image

image

image

image

image

image

Show resolved Hide resolved frontend/apps/reader/modules/readerlink.lua Outdated
@@ -746,7 +746,9 @@ function ReaderGesture:gestureAction(action, ges)
elseif action == "latest_bookmark" then
self.ui:handleEvent(Event:new("GoToLatestBookmark"))
elseif action == "follow_nearest_link" then
self.ui:handleEvent(Event:new("GoToPageLink", ges, false, G_reader_settings:isTrue("footnote_link_in_popup")))
self.ui:handleEvent(Event:new("GoToPageLink", ges,
G_reader_settings:nilOrTrue("swipe_ignore_external_links"),

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 2, 2019

Member

I think defaulting to ignoring is an unintuitive default.

Suggested change
G_reader_settings:nilOrTrue("swipe_ignore_external_links"),
G_reader_settings:isTrue("swipe_ignore_external_links"),

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 2, 2019

Author Contributor

I disagree for swipe, which is there for a few years and evoke the physical gesture of turning pages to jump to footnotes and back. Having it suddently throw a popup is not a good thing.
(People like you who would still want that can now activate it - but no one else but you complained it was missing :)

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 2, 2019

Author Contributor

Well, from the multiswipe gesture, it could be another default or setting. But I don't feel like adding yet another setting for that. So let's have it the same as in readerlink.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 2, 2019

Member

It breaks the concept of links as well as the internal logic for ignoring external links.

For the gesture manager you could just make two actions: the current one that follows all links, and a new one called follow_nearest_internal_link.

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 2, 2019

Author Contributor

Ok, went with 2 actions, hardcoded false and hardcoded true, not using the single swipe ignore setting.

Show resolved Hide resolved frontend/apps/reader/modules/readerlink.lua
Links menu and handling tweaks
- Removed "Swipe to follow first link on page" menu item and
  handling code, as it feels not really as practical as
  "Swipe to follow nearest link".
- Removed recently added "External link action", as we can
  just always present a popup with the url and the available
  actions.
- Generic handling of these actions in onGoToExternalLink(),
  so they are proposed on the Wikipedia lookup popup too.
- Allow external link on PDF documents (previously, only
  internal links were handled).
- Have "Ignore external links on tap" available on all
  document types.
- Added "Ignore external links on swipe" (default to true,
  the current behaviour).
- Added multiswipe gesture "Follow nearest internal link"
  (the existing "Follow nearest link" now follows the
  nearest external or internal link)
- ButtonDialogTitle: added an option to look a bit more
  alike ConfirmBoxes.
- Footnote popups: fix link unhighlight when tap on external link.

@poire-z poire-z force-pushed the poire-z:links_tweaks branch from 7f116b9 to 85375de Apr 2, 2019

@Frenzie Frenzie merged commit 5c38bcb into koreader:master Apr 2, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Frenzie Frenzie added the UX label Apr 2, 2019

@Frenzie Frenzie added this to the 2019.04 milestone Apr 2, 2019

@poire-z poire-z deleted the poire-z:links_tweaks branch Apr 2, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Would it look a bit better, more balanced, with the text centered (with just adding title_align = "center") ?

image

image

image

image

image

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

I don't think it does. Better, that is.

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

I like it for actual titles, not so much for multiline links, FWIW ;).

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

It's okay for titles, yes. Not really better or worse. But the link is both more balanced and also much worse. ;-)

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

Been toying with adding some kind of PTF (Poor Text Format) to TextBoxWidget, so we could specify alignment hints on individual lines in text fed to TextBoxWidget.
With some bits like:

@@ -36,6 +36,8 @@ local TextBoxWidget = InputContainer:new{
     editable = false, -- Editable flag for whether drawing the cursor or not.
     justified = false, -- Should text be justified (spaces widened to fill width)
     alignment = "left", -- or "center", "right"
+    simple_formatting = false, -- Set to true to look for simple formatting hints via chars
+                               -- (do not enable it when editable=true, not tested)

+-- Simple formatting chars (picked from Unicode Private Use area)
+
+-- Line alignment (that's all we have for now)
+TextBoxWidget.LINE_ALIGN_LEFT = "\xEE\x90\x80" -- U+E400
+TextBoxWidget.LINE_ALIGN_CENTER = "\xEE\x90\x81" -- U+E401
+TextBoxWidget.LINE_ALIGN_RIGHT = "\xEE\x90\x82" -- U+E402
+TextBoxWidget.LINE_ALIGN_JUSTIFY = "\xEE\x90\x83" -- U+E403
[+ some added code to deal with them]

and:

-        text = T(_("Would you like to read this Wikipedia %1 article?\n\n%2\n"), wiki_lang:upper(), wiki_page:gsub("_", " "))
+        simple_formatting = true
+        local TextBoxWidget = require("ui/widget/textboxwidget")
+        text = T(_("Would you like to read this Wikipedia %1 article?\n\n%3%2\n"), wiki_lang:upper(), wiki_page:gsub("_", " "), TextBoxWidget.LINE_ALIGN_CENTER)
         if epub_fullpath then
-            text = T("%1\n%2", text, _("This article has previously been saved as EPUB. You may wish to read the saved EPUB instead."))
+            text = T("%1\n%3%2", text, _("This article has previously been saved as EPUB. You may wish to read the saved EPUB instead."), TextBoxWidget.LINE_ALIGN_JUSTIFY)
[...]
     dialog = ButtonDialogTitle:new{
         title = text,
+        simple_formatting = simple_formatting,
         use_info_style = true,
         buttons = buttons,
     }

We could get when needed results like this (just an example, we'd probably only use LINE_ALIGN_CENTER on the Wikipedia title in that specific example):

image

But I wonder:

  • it sucks a bit to have to use templates to insert them, and how to present/explain them in the localized strings
  • could we just use more obvious single ASCII chars to hint that alignment? I don't see anything obvious ([<=_~).
    (it has to be single chars for easy parsing in TextBoxWidget)
@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

The way that looks is not great for localization purposes, to put it almost euphemistically. ;-)

If you want to experiment with typography, why not just use the rich text widget we have at our disposal these days? No centering, just something like a simple <strong></strong>, possibly combined with a little margin:0 1em. After having achieved a general look and functionality, you can switch to Markdown instead for simpler internationalization.

Also see, e.g., #2555 (comment) although I think that "Are you sure you want to wipe" might be going a bit overboard. Mainly I'd just make the first line bold:

Screenshot_2019-04-06_13-03-15

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

From the Gnome HIG, which aligns with my comment above:

Avoid the use of italic or oblique faces, as these are visually more complex, and can be distracting.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

The way that looks is not great for localization purposes

That's why I asked for alternative suggestions.
It could come down to, if using = char for centering:

text = T(_("Would you like to read this Wikipedia %1 article?\n\n=%2\n"),
                   wiki_lang:upper(), wiki_page:gsub("_", " "))

Or, if hardcoding the layout:

text = T(_("Would you like to read this Wikipedia %1 article?"), wiki_lang:upper()) ..
                   "\n\n" .. TextBoxWidget.LINE_ALIGN_CENTER .. wiki_page:gsub("_", " "))

If you want to experiment with typography

I don't :) Just wanted a quick solution to this not that nice display of the Wikipedia article title.
It's just that if they were already some kind of simple embedded line formatting syntax from the 1990s, simpler that Markdow, we could justify supporting that, instead of my unicode private use chars.
But I'm not that fond of my TextBoxWidget tweaks. No problem just forgetting it.

why not just use the rich text widget

Not that keen on having to bring MuPDF to render simple text messages. And using HTML in the localisation strings is prone to errors, unbalanced tags, readability...

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Not that keen on having to bring MuPDF to render simple text messages. And using HTML in the localisation strings is prone to errors, unbalanced tags, readability...

I am because I'm not talking about simple, although even merely implementing support for **strong** would suboptimally improve most current use cases. It would still be a relatively limited number either way.

You shouldn't use HTML, but Markdown.

Blockquote

Would you like to read this Wikipedia %1 article?

> %2

Strong

Would you like to read this Wikipedia %1 article?

**%2**

List

Would you like to read this Wikipedia %1 article?

* %2

Some header

Would you like to read this Wikipedia %1 article?

## %2

Pre

Would you like to read this Wikipedia %1 article?

```
%2
```

Also note definition lists (here, might make it into CommonMark soonish?), although like some of the others mentioned probably not so much for this use case

Would you like to read this Wikipedia %1 article?
:    %2

The specifics don't necessarily matter too much. Ideally the element base style suffices, but you can pass along some minor adjustments in the stylesheet.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

But you and markdown do not even answer my original wish: text centering :( :)

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

I don't agree that it should be centered. I think it looks unbalanced in combination with a ragged-right edge. NB I don't think "balanced" is a particularly important metric, unless all else is roughly equal.

Weight and size are the best way to improve clarity of the dialogs. However, the above is most definitely the answer to your question and not my vision on what it should look like. See below for that. Because no matter which element you go for, if you tell it to text-align:center that's what it'll do. ;-)

I think blockquote or emphasized does a better job.

Screenshot_2019-04-06_17-15-26
Screenshot_2019-04-06_17-18-57


Screenshot_2019-04-06_17-14-13
Screenshot_2019-04-06_17-17-27


Screenshot_2019-04-06_17-19-58

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.