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

Added choice of default action when highlighting #4486

Merged
merged 7 commits into from Jan 17, 2019

Conversation

Projects
None yet
4 participants
@Galunid
Copy link
Contributor

Galunid commented Jan 16, 2019

This resolves #4480. Adds Action on highlight subsection in Document> menu, that allows for using default action when user highlights multiple words.

@Frenzie
Copy link
Member

Frenzie left a comment

Probably looks okay, but I'm a bit distracted by the indentation. ;-)

}
},
{
text = _("Action on highlight"),

This comment has been minimized.

@Frenzie

Frenzie Jan 16, 2019

Member

There are too many spaces here.

Suggested change Beta
text = _("Action on highlight"),
text = _("Highlight action"),
end,
},
{
text = _("Check wikipedia"),

This comment has been minimized.

@Frenzie

Frenzie Jan 16, 2019

Member
Suggested change Beta
text = _("Check wikipedia"),
text = _("Search Wikipedia"),

Or just plain Wikipedia or Wikipedia lookup.

tap_close_callback = function() self:handleEvent(Event:new("Tap")) end,
}
UIManager:show(self.highlight_dialog)
default_highlight_action = G_reader_settings:readSetting("default_highlight_action")

This comment has been minimized.

@Frenzie

Frenzie Jan 16, 2019

Member

Four spaces, not tabs.

This comment has been minimized.

@poire-z

poire-z Jan 16, 2019

Contributor

Will also need a local to please luacheck.

text = _("Action on highlight"),
sub_item_table = {
{
text = _("Default"),

This comment has been minimized.

@Frenzie

Frenzie Jan 16, 2019

Member
Suggested change Beta
text = _("Default"),
text = _("Default (ask)"),
@poire-z
Copy link
Contributor

poire-z left a comment

Looks good, except tabs :)

tap_close_callback = function() self:handleEvent(Event:new("Tap")) end,
}
UIManager:show(self.highlight_dialog)
default_highlight_action = G_reader_settings:readSetting("default_highlight_action")

This comment has been minimized.

@poire-z

poire-z Jan 16, 2019

Contributor

Will also need a local to please luacheck.

self:onClose()
elseif default_highlight_action == "wikipedia" then
self:lookupWikipedia()
self:onClose()

This comment has been minimized.

@poire-z

poire-z Jan 16, 2019

Contributor

Please just check it does not crash or behave too badly with these self:onClose() (which will just clear the highlight).
(In the original callbacks, there are a few We don't call self:onClose() <reason why>

return setting == "ask" or setting == nil
end,
callback = function()
G_reader_settings:saveSetting("default_highlight_action", "ask")

This comment has been minimized.

@poire-z

poire-z Jan 16, 2019

Contributor

Ideally, you could just use nil, no real need for "ask".
G_reader_settings:saveSetting("default_highlight_action", nil) will remove the setting and make things cleaner.

@Galunid

This comment has been minimized.

Copy link
Contributor Author

Galunid commented Jan 16, 2019

I fixed indentation, can I change this pull request, or should I just close it and send another one?

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Jan 16, 2019

Just add another commit to the PR. :-)

@Galunid

This comment has been minimized.

Copy link
Contributor Author

Galunid commented Jan 16, 2019

Running shfmt on kodev
Exited with code 1

This seems strange, and unrelated to this PR, am I missing something?

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Jan 16, 2019

When you push a PR, some standard syntax/cleanliness checks are done. Like shfmt and luacheck.
They report stuff on your code. Look at the details to see the bottom of the full output:
The aim is zero warning :)

Luacheck results
Checking frontend/apps/reader/modules/readerhighlight.lua 6 warnings

    frontend/apps/reader/modules/readerhighlight.lua:525:9: setting non-standard global variable 'default_action_when_highlighting'
    frontend/apps/reader/modules/readerhighlight.lua:526:12: accessing undefined variable 'default_action_when_highlighting'
    frontend/apps/reader/modules/readerhighlight.lua:526:55: accessing undefined variable 'default_action_when_highlighting'
    frontend/apps/reader/modules/readerhighlight.lua:619:16: accessing undefined variable 'default_action_when_highlighting'
    frontend/apps/reader/modules/readerhighlight.lua:622:16: accessing undefined variable 'default_action_when_highlighting'
    frontend/apps/reader/modules/readerhighlight.lua:625:16: accessing undefined variable 'default_action_when_highlighting'

Just add a local before default_action_when_highlighting and these are gone.
And check our comments for other wishes :)

(You don't need to revert your previous commit - just correct, git add and commit your updated files, and git push origin yourbanche)

@@ -317,6 +317,48 @@ common_settings.document = {

}
},
{
text = _("Default action when highlighting"),

This comment has been minimized.

@Frenzie

Frenzie Jan 17, 2019

Member
Suggested change Beta
text = _("Default action when highlighting"),
text = _("Highlight action"),
Device.input.setClipboardText(self.selected_text.text)
end,
},
local default_action_when_highlighting = G_reader_settings:readSetting("default_highlight_action")

This comment has been minimized.

@Frenzie

Frenzie Jan 17, 2019

Member
Suggested change Beta
local default_action_when_highlighting = G_reader_settings:readSetting("default_highlight_action")
local default_highlight_action = G_reader_settings:readSetting("default_highlight_action")
text = _("Default action when highlighting"),
sub_item_table = {
{
text = _("Default(show menu)"),

This comment has been minimized.

@Frenzie

Frenzie Jan 17, 2019

Member
Suggested change Beta
text = _("Default(show menu)"),
text = _("Ask with popup dialog"),
end,
},
local default_highlight_action = G_reader_settings:readSetting("default_highlight_action")
if default_highlight_action == nil or default_highlight_action == "ask" then

This comment has been minimized.

@Frenzie

Frenzie Jan 17, 2019

Member
Suggested change Beta
if default_highlight_action == nil or default_highlight_action == "ask" then
if not default_highlight_action then
},
local default_highlight_action = G_reader_settings:readSetting("default_highlight_action")
if default_highlight_action == nil or default_highlight_action == "ask" then
logger.dbg("show highlight dialog")

This comment has been minimized.

@Frenzie

Frenzie Jan 17, 2019

Member

We can probably just leave this one out?

Suggested change Beta
logger.dbg("show highlight dialog")
{
text = _("Ask with popup dialog"),
checked_func = function()
return G_reader_settings:readSetting("default_highlight_action") == nil

This comment has been minimized.

@Frenzie

Frenzie Jan 17, 2019

Member
Suggested change Beta
return G_reader_settings:readSetting("default_highlight_action") == nil
return G_reader_settings:nilOrFalse("default_highlight_action")
@Frenzie

This comment has been minimized.

Please do use double quotes though. ;-)

},
})
}
if self.selected_link ~= nil then

This comment has been minimized.

@Frenzie

Frenzie Jan 17, 2019

Member

Sorry, I missed this one before. You can just write:

Suggested change Beta
if self.selected_link ~= nil then
if self.selected_link then
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Jan 17, 2019

@poire-z Any other comments? If not, please go ahead and merge. :-)

@poire-z poire-z merged commit 0678d75 into koreader:master Jan 17, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Jan 17, 2019

Looks good. Thanks!

@arooni

This comment has been minimized.

Copy link

arooni commented Feb 7, 2019

just wanted to say thank you to everyone on this. was looking for a way to make highlight a default, and voila; it's already there 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment