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

[feat, UX] Add external link to Wallabag #4863

Merged
merged 8 commits into from Mar 31, 2019

Conversation

Projects
None yet
2 participants
@Frenzie
Copy link
Member

commented Mar 31, 2019

Offers a choice when Wallabag is available.

Android/desktop (with Wallabag enabled)

Screenshot_2019-03-31_15-39-29

Other platforms (with Wallabag enabled)

Screenshot_2019-03-31_15-56-03

[feat, UX] Add external link to Wallabag
Offers a choice when relevant, i.e., on platforms that support an external browser.

@Frenzie Frenzie added the UX label Mar 31, 2019

@Frenzie Frenzie added this to the 2019.04 milestone Mar 31, 2019

@Frenzie Frenzie requested a review from poire-z Mar 31, 2019

@poire-z
Copy link
Contributor

left a comment

OK with the reorganizing.

@@ -576,56 +628,60 @@ function ReaderLink:onGotoLink(link, neglect_current_location, allow_footnote_po
})
end
return true
end
elseif self.ui.wallabag then
local settings = G_reader_settings:readSetting("external_link_action")

This comment has been minimized.

Copy link
@poire-z

poire-z Mar 31, 2019

Contributor

Forgot to add the menu item to set this setting? Or just undocumented for now?
(local settings = plural seems like a bad name for that variable :)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Mar 31, 2019

Author Member

I copied this from ReaderStatus. I wouldn't have put the settings in yet if I'd written it from scratch.

display_filename = display_filename .. anchor
},
}
if Device:openLink() == nil then

This comment has been minimized.

Copy link
@poire-z

poire-z Mar 31, 2019

Contributor

Calling Device:openLink() to know if it's supported seems a bit twisted. It returns false when set to no, and returns nil when it is supported, but provided with some invalid input...
If there's no cleaner way, at least if Device:openLink() ~= false then ?

This comment has been minimized.

Copy link
@Frenzie

Frenzie Mar 31, 2019

Author Member

Not defining it as no and testing Device.openLink?

This comment has been minimized.

Copy link
@poire-z

poire-z Mar 31, 2019

Contributor

Dunno. Is there already functions not defined in generic/device.lua that are defined in others?
I thought the idea of them being no was some kind of API listing everything that can be available.
Or add Device:canOpenLink = yes|no ?

This comment has been minimized.

Copy link
@Frenzie

Frenzie Mar 31, 2019

Author Member

I think it's mainly that you can safely call it and be sure it won't crash. API listing is more like a happy side effect.

Having a separate canOpenLink seems a touch inelegant but I suppose that'll have to be the way forward. :-)

})

choose_action = ButtonDialogTitle:new{
title = T(_("External link:\n\n'%1'"), link_url),

This comment has been minimized.

Copy link
@poire-z

poire-z Mar 31, 2019

Contributor

Not fond of the url inside ' ' on the screenshots. Can't you just let it plain?

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

I added a new menu item to the Links submenu. Looking at the screenshots below I realized it might be redundant with that ignore external links thing.

Screenshot_2019-03-31_17-23-18
Screenshot_2019-03-31_17-23-27

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

I added a new menu item to the Links submenu. Looking at the screenshots below I realized it might be redundant with that ignore external links thing.

I think it should be just below Ignore external links (as the swipe gestures currently ignore external links).
Or merged with it (but then "Ignore" instead of "Do nothing").
But on my Kobo with wallabag not enabled, it just serves as ignoring Wikipedia article links in Wikipedia EPUB for easier page change with Tap.
And ideally, the menu item should show the action selected, ie: External links: ignore > :)

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

I think it should be just below Ignore external links (as the swipe gestures currently ignore external links).

Speaking of which, why is that so immensely complicated? :-P

Hm, making it a little bit harder still there:

        -- (footnote popup, larger tap area and ignore external links
        -- are for now not supported with non-CreDocuments)
@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

Screenshot_2019-03-31_17-51-39
Screenshot_2019-03-31_17-53-11

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

why is that so immensely complicated

It's not: you just haven't done it yet, and I just haven't requested yet it to be a toggable :) and probably another one from the Tap one - as it involves a bit of expensive work in crengine, but all the work in crengine/cre.cpp is already done, you just have to make that hardcoded true a variable:

-- Getting segments on a page with many internal links is
-- a bit expensive. With larger_tap_area_to_follow_links=true,
-- this is done for each tap on screen (changing pages, showing
-- menu...). We might want to cache these links per page (and
-- clear that cache when page layout change).
-- As we care only about internal links, we request them only
-- (and avoid that expensive segments work on external links)
local links = self.ui.document:getPageLinks(true) -- only get internal links

and a bit of tweak at the end of ReaderLink:onGoToPageLink() to not do the xpointer checking when there is none but a uri.

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

I'm not really sure what you're asking tbh. I don't really understand the rationale behind what that setting does on closer examination. This probably comes back to my lack of understanding for why swipe to follow links is ignoring links. ;-)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

In Wikipedia EPUB, small footnotes are hard to tap on, and may be sitting just between 2 wikipedia URL links. Chances of taping ot swiping on it would be very low. Having avoid external links available makes that a non-issue.

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

I see. :-)

Anyhow, I think this PR now includes a response to all comments? (Other than dynamic menu text.)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

May be give that External link action some enabled_func like if self.ui.document.info.has_pages or not isTapIgnoreExternalLinksEnabled?

Otherwise lgtm.
(Except that now, that menu has 11 items and spans 2 pages :) I'll probably kill "Swipe to follow first link on page" as I don't see why one would use it since we have "Swipe to follow nearest link").

Frenzie added some commits Mar 31, 2019

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

vcfrtdggggggggg ← that's my cat's response

@Frenzie Frenzie merged commit 709207a into koreader:master Mar 31, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Frenzie Frenzie deleted the Frenzie:add-to-wallabag branch Mar 31, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

I've been looking at this to see how we could merge Ignore external links into the added External link action> submenu (and adding the selected action text into the main menu text).
I wondered if we should keep both "ignore" (let the tap propagate) and "do nothing" (eat the tap, don't propagate).

But after some time, I just wonder if this menu is really useful with all the actions.
I guess most will choose only "ignore" or "ask with popup" - just to see the url before getting into the wallabag wifi popup and download, or launching an external browser. These are just one tap away anyway.

We could be just fine with the existing single checkbox Ignore external links: if checked, let the tap propagate to change page or find an internal link near - if unchecked, just show the popup with the available actions?

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

I'd never choose "nothing" and I don't think add to Wallabag makes all that much sense as a default, but I can definitely see opting for "open in browser" without disabling the Wallabag plugin.

PS It's only got this configuration because you requested it! :-P

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Forgot to add the menu item to set this setting? Or just undocumented for now?

I copied this from ReaderStatus

So, we have that menu because of that setting that just got there because it came along in the copying ? :)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Or if we were to keep it, it may be better to have it as a real submenu with various combinable items, instead of an alternative single choice one.
External links>

  • Ignore on tap
  • Ignore on swipe
  • Open in browser without asking
  • Open Wikipedia links also in browser without asking (avoid our preview in textboxwidget if you prefer your browser)
@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

So, we have that menu because of that setting that just got there because it came along in the copying ? :)

Well no, but I wouldn't have added a menu for it this weekend because studying the menu and the code related to it is an entirely different task. :-P The open in browser setting is obvious but not terribly relevant on most platforms, the "nothing" setting is the kind of thing you like (and easy), and add to Wallabag is the reason I added this in the first place so it would be odd if it weren't a setting. It's the menu you get if you want a menu now rather than next month. ;-)

it may be better to have it as a real submenu with various combinable items, instead of an alternative single choice one.

Naturally. ;-)

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.