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

Add subtitles in preferences #560

Merged
merged 2 commits into from
Oct 30, 2023
Merged

Add subtitles in preferences #560

merged 2 commits into from
Oct 30, 2023

Conversation

sstendahl
Copy link
Owner

@sstendahl sstendahl commented Oct 30, 2023

Adds subtitles to the first two options in the preferences, basically due to comments from @bertob that the options were a bit unclear. https://gitlab.gnome.org/Teams/Circle/-/issues/185

Perhaps the last two also need some slight explanation. They seemed somewhat clear to me (at least "Hide unselected items" seemed OK to me personally), but to a new user they may be a bit unclear at first glance. So I'm open to suggestions to clarify these options somewhat.

model: StringList{
strings [_("Center at maximum Y value"), _("Center at middle coordinate")]
strings [_("Center at maximum Y value"), _("Center at middle X value")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this we also need to update the key in the gschema as well as add logic in migration.py to handle the changed name

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason for this is that "Center at middle coordinate" is just long enough that it cuts off the last part of the sentence, usign "X value" instead is consistent with the other option and is a bit shorter. But I think you are correct that this logic should applied in the gschema and migration.py as well. Should be a bit careful of adding too much overhead to the migration.py and not change strings when not needed, so I'm open to keeping this string as-is for now and re-evaluate all strings later in a seperate PR before next release.

In this specific case, I just checked, and it seems to work fine without any new logic in migration.py. When I set it to "center at max Y" in Graphs 1.6, it is still at "center at max Y" in this commit. And when I set it to "center at middle X" in graphs 1.6, it is still at "center at middle X" in this latest commit.

This could be because "enum 0" (center at max Y) is unchanged in string, so that setting will be nicely copied over. And "enum 1" (center at middle X) is the default. So when reverting to this default value with this changing key, it still ends up correctly. I maybe need to think about it to be sure, but I think in this specific case new migration logic would not be necessary.

@cmkohnen
Copy link
Collaborator

The problem is that in migrate.py we don't differentiate between string and enum. Simply adding a check of if vaalue == *old_value* should suffice. I wouldn't worry too much about overhead as it only gets called once when the new version is launched. If the config folder doesn't exist it is simply skipped.

@sstendahl
Copy link
Owner Author

Yeah I tend to agree with you about overhead not being the biggest issue probably.

In this particular case however, the thing is that the keyfile is only relevant for non-default settings. Values that are not set there, or are wrong, just revert to the default. This means that whenever this key is equal to "Center at middle coordinate", the settings is not copied over due to the surpression of KeyError in this logic:

def _migrate_config(settings_, config_file):
    config = file_io.parse_json(config_file)
    for old_key, (category, key) in CONFIG_MIGRATION_TABLE.items():
        with contextlib.suppress(KeyError):
            settings = settings_.get_child(category)
            value = config[old_key]
            if "scale" in key:
                value = value.capitalize()
            if isinstance(value, str):
                settings.set_string(key, value)
            elif isinstance(value, bool):
                settings.set_boolean(key, value)
            elif isinstance(value, int):
                settings.set_int(key, value)
    config_file.delete(None)

The settings.set_string(key, value) line will raise a KeyError and it continues to the next value. This means that the setting for the center action behaviour is not set. Whenever that is the case, the default value is simply used, which just happens to be "Center at middle X value". If "Center at maximum Y value" was set, then this does get copied over because there's no KeyError. So also that case works just fine. So in this particular case, adding a seperate condition is redundant, at least from a user-facing perspective. There is a warning raised during migration: (process:2): GLib-GIO-WARNING **: 15:45:01.672: g_settings_set_value: value for key 'center' in schema 'se.sjoerd.Graphs.general' is outside of valid range, but that only happens once.

The same holds in general. If wrong values are set in the keyfile (~/.var/app/se.sjoerd.Graphs/config/glib-2.0/settings), then the default values are used for those settings. The keys get overwritten as soon as another value is set. I am not opposed to adding a condition to _migrate_config anyway just for good practice. But strictly it is not needed, while it does make the function a bit more complex.

There's a bit of code in migrate.py that does not seem to get called at all by the way. Basically the PlotSettings class and Item class in migrate (including all the associated migration tables) are never called. Eventually it will be useful to have some migration tables when the gschema keys do change. But compared to last release we don't have any old gschema keys, since previous release (1.6.1) still used a json for the settings.

@cmkohnen cmkohnen merged commit 2994aa2 into main Oct 30, 2023
6 checks passed
@sstendahl sstendahl deleted the subtitles_preferences branch November 15, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants