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

SX editor - use integer-type schema parameters rather than string and… #2046

Open
wants to merge 1 commit into
base: stable
Choose a base branch
from

Conversation

gjanssens
Copy link
Member

… double

GnuCash expects these values to be integers anyway, so this avoids needless type conversions

… double

GnuCash expects these values to be integers anyway, so this avoids needless type conversions
Comment on lines +588 to +594
auto num_months = gnc_prefs_get_int (GNC_PREFS_GROUP_SXED, GNC_PREF_NUM_OF_MONTHS);
if (num_months == 0)
{
PWARN ("Got invalid value '0' for number of months to display. This suggests a gsettings configuration issue. Continuing with a default value of 12 instead.");
num_months = 12;
}
gnc_dense_cal_set_num_months (priv->gdcal, num_months);
Copy link
Member

Choose a reason for hiding this comment

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

This fixes Bug 799470 so please use a bugfix commit message.

But number-of-months doesn't show up in dconf in Linux nor org.gnucash.GnuCash.plist on mac, there's no setting for it in Preferences>Scheduled transactions, and changing it in an SX Editor tab doesn't persist if I close the tab and open a new one so I'm a bit mystified about why it works and why it failed for the reporter of that bug.

Copy link
Member

Choose a reason for hiding this comment

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

From[Bug 799470 Comment 15] https://bugs.gnucash.org/show_bug.cgi?id=799470#c15):

The code to store the number of months is only called when you select View->Save layout as default.

That explains it: After Save layout number-of-months is in org.gnucash.GnuCash.plist.

Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes Bug 799470 so please use a bugfix commit message.

I'm not sure yet this fixes the bug. It does indeed prevent the crash, but as I explain on the bug the crash is the result of not properly being able to access the schema in the first place. We haven't figured out yet why that happens for the bug reporter. If we do need to add code to fix that part as well, I'll use the bugfix commit message for it.

Copy link
Member

Choose a reason for hiding this comment

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

OK, no harm in waiting.

@Bob-IT
Copy link
Contributor

Bob-IT commented Nov 30, 2024

Sorry for the inconvenience this has caused, it took a while to remember why I used those settings, When I first looked at implementing this I added a couple of default preference settings using a spin button for the panel setting and a combo for the month, these two widgets were giving a double and text as values and that is why I chose them. I then thought of adding the menu action 'Save Layout as default' which was simpler and decided to drop the preference changes as they were not realy needed but unfortunately did bot revisit the choices for the settings.

These values are only saved by the menu action and used when the schedule editor page is created. When Gnucash is closed with that page open, the values are saved as part of the gcm page settings.

I do not think I have ever tested when the key did not exist, did not think it was possible.

By changing the gschemas, does that reset the two values to default entries ?
I do not think it is a problem for these entries, unfortunately I have not tried it as I broke my PC.

@gjanssens
Copy link
Member Author

Sorry for the inconvenience this has caused, it took a while to remember why I used those settings, When I first looked at implementing this I added a couple of default preference settings using a spin button for the panel setting and a combo for the month, these two widgets were giving a double and text as values and that is why I chose them. I then thought of adding the menu action 'Save Layout as default' which was simpler and decided to drop the preference changes as they were not realy needed but unfortunately did bot revisit the choices for the settings.

Ah, that makes sense.

I do not think I have ever tested when the key did not exist, did not think it was possible.

As far as I know that can only happen if the wrong schema is loaded at runtime. Schemas are compiled and installed by our build scripts, but read by gsettings based on the paths found in XDG_DATA_DIRS. If where gnucash installs them is different from where gsettings is looking for them you may get mismatches and hence preference read/write issues. gsettings is quite blunt at this and simply aborts the running program. We have written some code around it to first test for a key and only if it's defined return it. Note that if a key is defined querying it will always return a value. Either whatever was set or the default value. I'm still puzzled how the reporter of bug 799470 gets into the situation where no value is returned from gsettings.

By changing the gschemas, does that reset the two values to default entries ? I do not think it is a problem for these entries, unfortunately I have not tried it as I broke my PC.

Thanks for asking, I did test before submitting this PR. By changing the type of the gsettings setting, the eventually already stored value is discarded and the default value of the new type is returned. So this seems to be safe to do (at least on Fedora 39).

@jralls
Copy link
Member

jralls commented Nov 30, 2024

By changing the type of the gsettings setting, the eventually already stored value is discarded and the default value of the new type is returned. So this seems to be safe to do (at least on Fedora 39).

Safe but annoying. Please add a notice about that in the commit message for the release notes.

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.

3 participants