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

Get the time format from the new key and make it more efficient #401

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tintou
Copy link
Member

@tintou tintou commented Jun 2, 2019

No description provided.

Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

I can't confirm that this works as expected. I always have 12h time in the sidebar no matter what my selection and I always have 24 time in the new event dialog (perhaps a bug in Granite?)

Screenshot from 2019-06-03 18 13 01@2x

@peteruithoven
Copy link
Collaborator

Looks like we'd need to use the same logic to set the format here:

Granite.Widgets.DatePicker make_date_picker () {
var format = Granite.DateTime.get_default_date_format (false, true, true);
var date_picker = new Granite.Widgets.DatePicker.with_format (format);
date_picker.width_request = 200;
return date_picker;
}

Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

There seems to be a slight regression with this branch. If the time-format is changed with the calendar open the format of existing events in the agenda does not change both in master and PR. However, in master if calendar is closed and reopened the format changes, but this did not happen with this PR. See screenshot after restarting Calendar (without logging out).
Screenshot from 2020-05-15 18 02 32

}
}

public string TimeFormat () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to change this function name to time_format () in order to resolve the conflict with master

if (clockformat == null)
return Granite.DateTime.get_default_time_format (true);
public bool is_12h { get; private set; default = false; }
private Greeter.AccountsService? greeter_act = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very minor point: I would call this greeter_acc or greeter_acnt` - as "act" is not generally used as an abbreviation for "account" (and is a word in its own right).

GLib.DBusProxyFlags.GET_INVALIDATED_PROPERTIES);
is_12h = greeter_act.time_format == "12h" ? true : false;
} catch (Error e) {
critical (e.message);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another minor point: Could add an indication that we are falling back to org.gnome.desktop.interface setting to the error message.

@jeremypw
Copy link
Collaborator

jeremypw commented May 5, 2021

@tintou Converting to draft as some outstanding issues and conflicts and no recent activity. Please feel free to close or reopen for review after fixing as required.

@jeremypw jeremypw marked this pull request as draft May 5, 2021 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants