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

Application: Add Calendar.Settings Singleton #733

Closed
wants to merge 10 commits into from

Conversation

Marukesu
Copy link
Contributor

Second part of elementary/settings-daemon#39.

This create an new Calendar.Settings class to provide both internal and external settings, while allowing calendar to work ever if they can't access any of the settings (eg. running ./build/src/io.elementary.calendar).

this is exposing two unused keys right now:

Maya.Settings was removed and time_format () moved there.

Marukesu and others added 10 commits December 18, 2021 07:49
This class replaces the old Maya.Settings namespace and provide access
to internal and external properties even when sandboxed.

For internal settings, this will use the GLib.Settings if disponible.
For external settings, this will first try to use the portal,
fallbacking to the GLib.Settings if it fails for any reason.
Also, remove the now unneeded settings from Application.vala
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.

Just a few comments.

The handling of WindowState seems a bit obscure - is this so it works with the portal?


// application settings
public DateTime date { get; set; }
public WindowState window { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would name this property as e.g. window_state since a window is already a thing.

string day = saved_state.get_string ("selected-day");
string month = saved_state.get_string ("month-page");

if ( day != "" || month != "") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if ( day != "" || month != "") {
if (day != "" || month != "") {

}

d = int.parse (split[1]);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be better to use string.scanf here together with the same format strings used write the settings?

}

private static DateWeekday parse_weekday_name (Variant variant) {
switch (variant.get_string ()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe force lower case as this might be fed user edited values?

@danirabbit danirabbit mentioned this pull request Jun 13, 2023
3 tasks
@zeebok
Copy link
Contributor

zeebok commented Jul 3, 2023

Some conflicts with main to sort through :)

@Marukesu
Copy link
Contributor Author

Marukesu commented Jul 3, 2023

Well, with the bakground portal not needing this anymore, and with the first_weekday PR closed, i think this PR lost purpose, for now.

unless the ability to run without the schemas installed is a much wanted feature, what i doubt it is.

@zeebok
Copy link
Contributor

zeebok commented Jul 6, 2023

That really only seems useful for some dev testing scenarios. Granted this was given to the OS 7.1 project and is part of the upcoming release draft so maybe @danirabbit can confirm if this should stay or not.

@danirabbit
Copy link
Member

Ah yeah i think this was on the project because at the time it was background portal related maybe? We can bump it off it doesn’t seem like it’s currently useful

@zeebok
Copy link
Contributor

zeebok commented Jul 6, 2023

Based on the last few messages it sounds like this can be closed.

@zeebok zeebok closed this Jul 6, 2023
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.

5 participants