search

accumulator / Quickddit

Reddit client for Jolla's SailfishOS, Ubuntu Touch and Nokia N9
GNU General Public License v3.0
53 stars 21 forks source link

Fix settings in SailJail #96

Closed DeathByDenim closed 1 year ago

DeathByDenim commented 1 year ago

This PR should close issue #95. Thanks in large part to @vigejolla. It also takes care of migration the settings from the old location to the new.

Thaodan commented 1 year ago

I think you can squash the second commit with the first.

I would also suggest to be more verbose "Fix locating settings, migrate the old settings" since the issues is finding the settings and then the migration of old changes no matter if in the sandbox or not.

Also it makes sense to prefix the commit with Sailfish: as the changes are Sailfish specific.

DeathByDenim commented 1 year ago

I think you can squash the second commit with the first.

I would also suggest to be more verbose "Fix locating settings, migrate the old settings" since the issues is finding the settings and then the migration of old changes no matter if in the sandbox or not.

Also it makes sense to prefix the commit with Sailfish: as the changes are Sailfish specific.

Sure, I can do that. I'll do that after the fix for the old config location.

DeathByDenim commented 1 year ago

I squashed the commits. Should be good now, I hope!

Thaodan commented 1 year ago

I squashed the commits. Should be good now, I hope!

Looks good to me however I wouldn't mention Sailjail as Sailjail isn't the trigger of the problem.

DeathByDenim commented 1 year ago

I squashed the commits. Should be good now, I hope!

Looks good to me however I wouldn't mention Sailjail as Sailjail isn't the trigger of the problem.

It's part of it though. X-Sailjail was misspelled as X-SailJail (case sensitivity). That caused the location to not be writable. This PR makes that change.

Thaodan commented 1 year ago

DeathByDenim @.***> writes:

Looks good to me however I wouldn't mention Sailjail as Sailjail isn't the trigger of the problem.

It's part of it though. X-Sailjail was misspelled as X-SailJail (case sensitivity). That caused the location to not be writable. This PR makes that change.

Makes sense!

Maybe the Sailjail change should be in a second commit since they are separate changes.

But that's just a nitpick.

DeathByDenim commented 1 year ago

Makes sense! Maybe the Sailjail change should be in a second commit since they are separate changes. But that's just a nitpick.

Ok, how's this? :)

accumulator commented 1 year ago

It's part of it though. X-Sailjail was misspelled as X-SailJail (case sensitivity). That caused the location to not be writable. This PR makes that change.

Ugh! Thanks, I've been banging my head on why it could save settings but not load them again. I have very little time to look at Quickddit a.t.m. so much appreciated!

Can I merge this?

vigejolla commented 1 year ago

Can I merge this?

I'm not sure who are you asking from, but I think this is good to merge :)

DeathByDenim commented 1 year ago

Oh yes, that spelling error is an awful one. I'm very glad @vigejolla caught that one! :) Thanks for merging!