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

Show current and default values in config dialog (epub) #3952

Merged
merged 5 commits into from May 18, 2018

Conversation

Projects
None yet
5 participants
@robert00s
Contributor

robert00s commented May 15, 2018

After long press on name of options in config dialog we can see now current and default value. For now only for epub (cre) documents.
Long press on "View Mode"
romeo and juliet - koreader_092

Long press on "Line Spacing"
romeo and juliet - koreader_093

Long press on "Margin"
romeo and juliet - koreader_094

@Frenzie

Looks useful. :-)

local function showValues(configurable, title ,setting_default, setting_curr, suffix, arg_string, true_values)
local default = G_reader_settings:readSetting(setting_default)
local current = configurable[setting_curr]
local value_defalut, value_current

This comment has been minimized.

@Frenzie

Frenzie May 15, 2018

Member

Although for the code it doesn't really matter I'd still prefer default to be spelled correctly. ;-)

end
UIManager:show(InfoMessage:new{
text = T(_("%1:\nCurrent value: %2\n left: %3\n top: %4\n right: %5\n bottom: %6\n" ..
"Defalut value: %7\n left: %8\n top: %9\n right: %10\n bottom: %11"),

This comment has been minimized.

@Frenzie

Frenzie May 15, 2018

Member

This user-facing typo is more important. But note that concat doesn't work for internationalization. For multiline please use [[]] if desired, which I thought would be clearer here regardless. :-)

local arg_view_mode = {
[0] = S.VIEW_PAGE,
[1] = S.VIEW_SCROLL,
}

This comment has been minimized.

@poire-z

poire-z May 15, 2018

Contributor

I'm not fond of these, so far away from the CreOptions that have this info somewhere in each of the sub-tables.
But as you already do that:

                    hold_callback = function()
                        if self.options[c].name_text_hold_callback then
                            self.options[c].name_text_hold_callback(self.config.configurable)
                        end
                    end,

may be you can give it the options itself, like:

                    hold_callback = function()
                        if self.options[c].name_text_hold_callback then
                            self.options[c].name_text_hold_callback(self.config.configurable, self.options[c])
                        end
                    end,

so you can use it directly in:

-                name_text_hold_callback = function(configurable)
-                    showValues(configurable, S.VIEW_MODE, "copt_view_mode", "view_mode", "", arg_view_mode)
+                name_text_hold_callback = function(configurable, option)
+                    showValues(configurable, S.VIEW_MODE, "copt_view_mode", "view_mode", "", option.toggle)
                end,

(untested, and may be you can even get other stuff from option to give to showValues instead of rewritting them).

This comment has been minimized.

@robert00s

robert00s May 15, 2018

Contributor

Nice :) I try it.

This comment has been minimized.

@poire-z

poire-z May 15, 2018

Contributor

Nice try :)

You could even get rid of the "copt_blahblah" , as we know how it's build in ConfigDialog:onMakeDefault:

            name = self.config_options.prefix.."_"..name
            G_reader_settings:saveSetting(name, values[position])

You can't reach CreOptions.prefix, but you could may be hardcode it in your showValues(), something like
local default = G_reader_settings:readSetting("copt_"..setting_curr)

The clearer would be being able to do:

                name_text_hold_callback = function(configurable, opt)
                    showValues(configurable, opt)
                 end,

which could be shorten to:

                name_text_hold_callback = showValues

This comment has been minimized.

@robert00s

robert00s May 16, 2018

Contributor

Thanks, very valuable advice :)

local function enable_if_equals(configurable, option, value)
return configurable[option] == value
end
local function showValues(configurable, title ,setting_default, setting_curr, suffix, arg_string, true_values)
local function showValues(configurable, title ,setting_default, setting_curr, suffix, arg_string, arg_values, true_values)

This comment has been minimized.

@poire-z

poire-z May 15, 2018

Contributor

title, setting_default (comma then space)

@cramoisi

This comment has been minimized.

Contributor

cramoisi commented May 16, 2018

@robert00s and the long-press would still set the value as default value ?

some days ago, I saw a screenshot with a star beside a entry of a menu and I thought it could be cool to not only set a default value but also have a visual thing to see if the value is already defaulted (like text italic, or underscored, don't know).

@Frenzie Frenzie changed the title from Show current and defalut values in config dialog (epub) to Show current and default values in config dialog (epub) May 16, 2018

@poire-z

This comment has been minimized.

Contributor

poire-z commented May 16, 2018

@cramoisi (as I wondered the same, before I looked at the code): this PR deals with a long-press on the title strings (which currently does nothing) on the left of the buttons (long-press on buttons is not changed, and will make them defaulted)

@cramoisi

This comment has been minimized.

Contributor

cramoisi commented May 16, 2018

@poire-z ok !

@KenMaltby

This comment has been minimized.

KenMaltby commented May 16, 2018

So a new (and useful) "Long press" protocol;

  1. long pressing on the option selection itself = set to default
  2. long pressing on the title of the setting = a display of the current and default option & values. Sounds like a lot of work, but would become a convenience for the user and fits a logical progression for the feature.
@robert00s

This comment has been minimized.

Contributor

robert00s commented May 16, 2018

@KenMaltby: exactly

local default = G_reader_settings:readSetting("copt_"..option.name)
local current = configurable[option.name]
local value_default, value_current
if not option.suffix then option.suffix = "" end

This comment has been minimized.

@poire-z

poire-z May 16, 2018

Contributor

Just to not add stuff to the option sub-table, you could use:
local suffix = option.suffix or ""
and use that local suffix below

@@ -65,6 +171,9 @@ local CreOptions = {
DCREREADER_CONFIG_LINE_SPACE_PERCENT_MEDIUM,
DCREREADER_CONFIG_LINE_SPACE_PERCENT_LARGE,
},
suffix = "%",
true_values = true,
name_text_hold_callback = showValues,

This comment has been minimized.

@poire-z

poire-z May 16, 2018

Contributor

I was first going to suggest to call these name_text_suffix and name_text_true_values so we know quickly that they are just used by name_text_hold_callback (and we don't go wondering if they are used by configdialog), but it would look ugly, and may be these suffix and true_values could be used later for other things.
May be you could just move them below name_text_hold_callback, and just add a comment on their right -- used by showValues ?
Looks clean overall :)

@poire-z

This comment has been minimized.

Contributor

poire-z commented May 16, 2018

Thanks, looks alright to me.

@robert00s

This comment has been minimized.

Contributor

robert00s commented May 18, 2018

@Frenzie: Can I merge?

@poire-z poire-z merged commit da65db1 into koreader:master May 18, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@KenMaltby

This comment has been minimized.

KenMaltby commented May 19, 2018

The "lot of work" I was referring to comes over time as the rest of the potential this approach presents. Long pressing on the titles/labels could bring up enough description or information to amount to an "inline user's manual". It may not answer the user's "How do I do x, y, or z?" but it could answer the question "What does this setting/menu item do?"

@poire-z

This comment has been minimized.

Contributor

poire-z commented May 19, 2018

@robert00s : a few observations:
Long press on Orientation shows: Current value: %2
Long press on Contrast shows the internal value (some index, from 10 to 56). It would be best if it would show the font gamma (0.8 to 15), which is what we display when hold on button or in the notification when changing with tap. I had to put them in an alternate label key:

{
name = "font_gamma",
name_text = S.CONTRAST,
buttonprogress = true,
default_value = 15, -- gamma = 1.0
default_pos = 2,
values = {10, 15, 25, 30, 36, 43, 49, 56},
event = "SetFontGamma",
args = {10, 15, 25, 30, 36, 43, 49, 56},
-- gamma values for these indexes are:
labels = {0.8, 1.0, 1.45, 1.90, 2.50, 4.0, 8.0, 15.0},
name_text_hold_callback = showValues,
},

@robert00s

This comment has been minimized.

Contributor

robert00s commented May 19, 2018

@poire-z I hope that (#3961) fix Orientation and Contrast.

@poire-z

This comment has been minimized.

Contributor

poire-z commented May 19, 2018

It does, thanks :)

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