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

set global settings independent of local setting #5522

Merged
merged 10 commits into from Oct 25, 2019
@@ -9,6 +9,7 @@ local InputDialog = require("ui/widget/inputdialog")
local JSON = require("json")
local KeyValuePage = require("ui/widget/keyvaluepage")
local LuaData = require("luadata")
local MultiConfirmBox = require("ui/widget/multiconfirmbox")
local NetworkMgr = require("ui/network/manager")
local Trapper = require("ui/trapper")
local UIManager = require("ui/uimanager")
@@ -249,7 +250,7 @@ If you'd like to change the order in which dictionaries are queried (and their r
self.disable_fuzzy_search = not self.disable_fuzzy_search
end,
hold_callback = function()
self:makeDisableFuzzyDefault(self.disable_fuzzy_search)
self:toggleFuzzyDefault()
end,
separator = true,
},
@@ -950,21 +951,23 @@ function ReaderDictionary:onSaveSettings()
self.ui.doc_settings:saveSetting("disable_fuzzy_search", self.disable_fuzzy_search)
end

function ReaderDictionary:makeDisableFuzzyDefault(disable_fuzzy_search)
logger.dbg("disable fuzzy search", self.disable_fuzzy_search)
UIManager:show(ConfirmBox:new{
function ReaderDictionary:toggleFuzzyDefault()
local disable_fuzzy_search = G_reader_settings:isTrue("disable_fuzzy_search")
UIManager:show(MultiConfirmBox:new{
text = T(
disable_fuzzy_search
and _("Disable fuzzy search by default?")
or _("Enable fuzzy search by default?")
and _("Enable fuzzy search by default?")
or _("Disable fuzzy search by default?")
This conversation was marked as resolved by yparitcher

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member

Keeping this or in place makes the dialog confusing. The "OK" answer to the question asked should always be the one on the right, so changing text around like this implies also swapping the buttons. But we should rewrite the text, not swap the buttons.

Taking a cue from what I wrote in #3030 (comment), we can take the opportunity to build in some documentation. Here's a quick draft:

Would you like to enable or disable fuzzy search by default? The current default (★) is enabled.

Fuzzy search is able to match epuisante, épuisante and épuisantes to épuisant, even if only the latter has an entry in the dictionary. It's normally recommended to keep fuzzy search enabled, but it does have a performance impact. If you're having trouble with performance, it might be worthwhile to look into disabling unneeded dictionaries before disabling fuzzy search.

But thinking about the translators before changing any text is a good instinct to have! ;-)

This comment has been minimized.

Copy link
@yparitcher

yparitcher Oct 24, 2019

Author Contributor

this pr is just for the defaults, you can add docs however you want

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 25, 2019

Member

I assume that comes across more brusque than intended, but I'm still a bit confused by your objection. This is my suggested string for this particular default in line with the rest of the UI, no more no less.

I'll merge it with the button disabling in place for now.

This comment has been minimized.

Copy link
@yparitcher

yparitcher Oct 25, 2019

Author Contributor

sorry, was not trying to be so harsh, i just wanted to get this out without getting tangled up in documentation. i have seen PRs (in other repos) that get sidetracked and stalled.

),
ok_text = T(
disable_fuzzy_search
and _("Disable")
or _("Enable")
),
ok_callback = function()
G_reader_settings:saveSetting("disable_fuzzy_search", disable_fuzzy_search)
choice1_text = _("Disable"),
choice1_enabled = not disable_fuzzy_search,
choice1_callback = function()
G_reader_settings:saveSetting("disable_fuzzy_search", true)
end,
choice2_text = _("Enable"),
choice2_enabled = disable_fuzzy_search,
choice2_callback = function()
G_reader_settings:saveSetting("disable_fuzzy_search", false)
end,
})
end
@@ -1,10 +1,10 @@
local ButtonDialog = require("ui/widget/buttondialog")
local ConfirmBox = require("ui/widget/confirmbox")
local Device = require("device")
local Event = require("ui/event")
local InfoMessage = require("ui/widget/infomessage")
local Notification = require("ui/widget/notification")
local InputContainer = require("ui/widget/container/inputcontainer")
local MultiConfirmBox = require("ui/widget/multiconfirmbox")
local Notification = require("ui/widget/notification")
local TimeVal = require("ui/timeval")
local Translator = require("ui/translator")
local UIManager = require("ui/uimanager")
@@ -116,7 +116,7 @@ function ReaderHighlight:genHighlightDrawerMenu()
self.view.highlight.disabled = not self.view.highlight.disabled
end,
hold_callback = function(touchmenu_instance)
self:makeDefault(not self.view.highlight.disabled)
self:toggleDefault()
end,
separator = true,
},
@@ -1179,17 +1179,20 @@ function ReaderHighlight:onClose()
self:clear()
end

function ReaderHighlight:makeDefault(highlight_disabled)
local new_text
if highlight_disabled then
new_text = _("Disable highlight by default.")
else
new_text = _("Enable highlight by default.")
end
UIManager:show(ConfirmBox:new{
text = new_text,
ok_callback = function()
G_reader_settings:saveSetting("highlight_disabled", highlight_disabled)
function ReaderHighlight:toggleDefault()
local highlight_disabled = G_reader_settings:isTrue("highlight_disabled")
UIManager:show(MultiConfirmBox:new{
text = highlight_disabled and _("Enable highlighting by default.")
This conversation was marked as resolved by yparitcher

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member

Same basic comment as above, but I assume highlighting is straightforward enough.

Would you like to enable or disable highlighting by default? The current default (★) is enabled.
or _("Disable highlighting by default."),
choice1_text = _("Disable"),
choice1_enabled = not highlight_disabled,
choice1_callback = function()
G_reader_settings:saveSetting("highlight_disabled", true)
end,
choice2_text = _("Enable"),
choice2_enabled = highlight_disabled,
choice2_callback = function()
G_reader_settings:saveSetting("highlight_disabled", false)
end,
})
end
@@ -1,9 +1,9 @@
local ConfirmBox = require("ui/widget/confirmbox")
local Device = require("device")
local Event = require("ui/event")
local Geom = require("ui/geometry")
local InputContainer = require("ui/widget/container/inputcontainer")
local Math = require("optmath")
local MultiConfirmBox = require("ui/widget/multiconfirmbox")
local UIManager = require("ui/uimanager")
local logger = require("logger")
local _ = require("gettext")
@@ -244,13 +244,20 @@ function ReaderPaging:addToMainMenu(menu_items)
self.ui:handleEvent(Event:new("ToggleReadingOrder"))
end,
hold_callback = function(touchmenu_instance)
UIManager:show(ConfirmBox:new{
text = self.inverse_reading_order and _("Enable right to left reading by default?")
or _("Disable right to left reading by default?"),
ok_text = self.inverse_reading_order and _("Enable")
or _("Disable"),
ok_callback = function()
G_reader_settings:saveSetting("inverse_reading_order", self.inverse_reading_order)
local inverse_reading_order = G_reader_settings:isTrue("inverse_reading_order")
UIManager:show(MultiConfirmBox:new{
text = inverse_reading_order and _("The default for newly opened books is Right To Left page turning.\nWould you like to change it?")
This conversation was marked as resolved by yparitcher

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member
Suggested change
text = inverse_reading_order and _("The default for newly opened books is Right To Left page turning.\nWould you like to change it?")
text = inverse_reading_order and _("The default (★) for newly opened books is right-to-left (RTL) page turning.\n\nWould you like to change it?")
or _("The default for newly opened books is Left To Right page turning.\nWould you like to change it?"),
This conversation was marked as resolved by yparitcher

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member
Suggested change
or _("The default for newly opened books is Left To Right page turning.\nWould you like to change it?"),
or _("The default (★) for newly opened books is left-to-right (LTR) page turning.\n\nWould you like to change it?"),
choice1_text = _("Left-To-Right"),
This conversation was marked as resolved by yparitcher

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member

You can use a text_func (or was that text_fun) to vary between LTR (★) and LTR.

choice1_enabled = inverse_reading_order,
choice1_callback = function()
G_reader_settings:saveSetting("inverse_reading_order", false)
if touchmenu_instance then touchmenu_instance:updateItems() end
end,
choice2_text = _("Right-To-Left"),
choice2_enabled = not inverse_reading_order,
choice2_callback = function()
G_reader_settings:saveSetting("inverse_reading_order", true)
if touchmenu_instance then touchmenu_instance:updateItems() end
end,
})
@@ -3,6 +3,7 @@ local ConfirmBox = require("ui/widget/confirmbox")
local Device = require("device")
local Event = require("ui/event")
local InputContainer = require("ui/widget/container/inputcontainer")
local MultiConfirmBox = require("ui/widget/multiconfirmbox")
local ProgressWidget = require("ui/widget/progresswidget")
local ReaderPanning = require("apps/reader/modules/readerpanning")
local TimeVal = require("ui/timeval")
@@ -370,13 +371,20 @@ function ReaderRolling:addToMainMenu(menu_items)
self.ui:handleEvent(Event:new("ToggleReadingOrder"))
end,
hold_callback = function(touchmenu_instance)
UIManager:show(ConfirmBox:new{
text = self.inverse_reading_order and _("Enable right to left reading by default?")
or _("Disable right to left reading by default?"),
ok_text = self.inverse_reading_order and _("Enable")
or _("Disable"),
ok_callback = function()
G_reader_settings:saveSetting("inverse_reading_order", self.inverse_reading_order)
local inverse_reading_order = G_reader_settings:isTrue("inverse_reading_order")
UIManager:show(MultiConfirmBox:new{
text = inverse_reading_order and _("The default for newly opened books is Right To Left page turning.\nWould you like to change it?")
or _("The default for newly opened books is Left To Right page turning.\nWould you like to change it?"),
choice1_text = _("Left-To-Right"),
choice1_enabled = inverse_reading_order,
choice1_callback = function()
G_reader_settings:saveSetting("inverse_reading_order", false)
if touchmenu_instance then touchmenu_instance:updateItems() end
end,
choice2_text = _("Right-To-Left"),
choice2_enabled = not inverse_reading_order,
choice2_callback = function()
G_reader_settings:saveSetting("inverse_reading_order", true)
if touchmenu_instance then touchmenu_instance:updateItems() end
end,
})
@@ -2,6 +2,7 @@ local ConfirmBox = require("ui/widget/confirmbox")
local Event = require("ui/event")
local InfoMessage = require("ui/widget/infomessage")
local InputContainer = require("ui/widget/container/inputcontainer")
local MultiConfirmBox = require("ui/widget/multiconfirmbox")
local UIManager = require("ui/uimanager")
local Math = require("optmath")
local lfs = require("libs/libkoreader-lfs")
@@ -428,14 +429,22 @@ function ReaderTypeset:addToMainMenu(menu_items)
end

function ReaderTypeset:makeDefaultFloatingPunctuation()
local toggler = self.floating_punctuation == 1 and _("On") or _("Off")
UIManager:show(ConfirmBox:new{
local floating_punctuation = G_reader_settings:isTrue("floating_punctuation")
local toggler = floating_punctuation and _("Disable") or _("Enable")
UIManager:show(MultiConfirmBox:new{
text = T(
_("Set default hanging punctuation to %1?"),
_("%1 hanging punctuation by default?"),
This conversation was marked as resolved by yparitcher

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member

That should be written out in full. There's no guarantee that enable and disable are interchangeable in other languages, let alone take the same form as on buttons without context.

However, it's a moot point because it should take the same form as the others:

Would you like to enable or disable hanging punctuation by default? The current default (★) is enabled.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member

As a concrete example, in Dutch it would be localized along these lines:

Button text

Inschakelen/Uitschakelen (Enable/Disable)

Or!

Schakel in/Schakel uit

Sentence

Hangende interpunctie standaard inschakelen (of uitschakelen)?

So even if the text itself were correct, we'd have a capitalization mismatch.

NB The above is intended as an illustration of idiomatic Dutch grammar (Enable X by default? = X standaard inschakelen?), not as an illustration of the best translation of the sentence in question.

This comment has been minimized.

Copy link
@yparitcher

yparitcher Oct 24, 2019

Author Contributor

i was trying to leave it as intact as possible, i didn't realize the implications.

toggler
),
ok_callback = function()
G_reader_settings:saveSetting("floating_punctuation", self.floating_punctuation)
choice1_text = _("Disable"),
choice1_enabled = floating_punctuation,
choice1_callback = function()
G_reader_settings:saveSetting("floating_punctuation", false)
end,
choice2_text = _("Enable"),
choice2_enabled = not floating_punctuation,
choice2_callback = function()
G_reader_settings:saveSetting("floating_punctuation", true)
end,
})
end
@@ -48,6 +48,8 @@ local MultiConfirmBox = InputContainer:new{
choice1_callback = function() end,
choice2_callback = function() end,
cancel_callback = function() end,
choice1_enabled = true,
This conversation was marked as resolved by yparitcher

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member

Looks like this doesn't support text_func atm? That's slightly unfortunate but it'd be easy to add. I assume the underlying Button(Table) already supports it.

choice2_enabled = true,
margin = Size.margin.default,
padding = Size.padding.default,
dismissable = true, -- set to false if any button callback is required
@@ -102,13 +104,15 @@ function MultiConfirmBox:init()
},
{
text = self.choice1_text,
enabled = self.choice1_enabled,
callback = function()
self.choice1_callback()
UIManager:close(self)
end,
},
{
text = self.choice2_text,
enabled = self.choice2_enabled,
callback = function()
self.choice2_callback()
UIManager:close(self)
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.