Screenshots housekeeping #56
Conversation
| var config_path = GLib.Path.build_filename ( | ||
| GLib.Environment.get_user_config_dir (), | ||
| var config_path = Path.build_filename ( | ||
| Environment.get_user_config_dir (), | ||
| "user-tmpfiles.d", | ||
| "io.elementary.settings-daemon.downloads-folder.conf" |
There was a problem hiding this comment.
I kept the same name of the tmpfiles config file. It is technically no longer correct, as it can now contain config for screenshots dir in addition to downloads. My thinking behind this:
- The implementation is simpler if we only have one file for all cleanup related config.
- If we wanted to change the file name we'd need to make sure to delete the old file when we upgrade the daemon. Without that the old file would drift out of sync with gsettings and keep causing problems. This "swap files on upgrade" logic seemed like an unnecessary complication, so I kept the name the same.
There was a problem hiding this comment.
Actually, if we ship this only with OS 7 we can get away with renaming the file, as the old one won't exist on a fresh install.
This ship has sailed. 😅
|
|
||
| if (clean_screenshots) { | ||
| var pictures_dir = Environment.get_user_special_dir (UserDirectory.PICTURES); | ||
| var screenshots_dir = Path.build_filename (pictures_dir, _("Screenshots")); |
There was a problem hiding this comment.
This _("Screenshots") here bothers me. The Screenshot app itself uses a translated directory name. That's why I used a translated string here as well. However, that leaves room for missing, or just inconsistent, translations (in either app) which will lead to incorrect cleanup behavior.
I guess this could be avoided if we somehow synced translations, but I'm not sure if that's possible (or how to do it). @danirabbit do you have any suggestions?
There was a problem hiding this comment.
I think there's a way to include the gettext domain here and use the translations from screenshot. I think we should actually be using the GNOME Settings daemon gettext domain here maybe? @tintou would probably know what to do about that
There was a problem hiding this comment.
I did some digging:
- I did not find a translation for
"Screenshots"in GNOME settings daemon. And if I'm reading its code correctly, I thing GNOME's default screenshot utility saves images straight into~/Pictures, without a dedicated screenshots sub-directory. - I noticed that we have screenshot saving implemented in 2 places: in Gala and in standalone Screenshot app.
The implementation differs slightly between Gala and Screenshot app: the app allows users to override the directory and will remember the new destination for later. On the other hand Gala implementation always uses the translated ~/Pictures/Screenshots directory.
Since Gala implementation looked more "stable" to me I opted for reusing that translation for this housekeeping feature. This effectively means that we'll "leak" screenshot images taken with the Screenshot app in 2 cases:
- If user overrides the default directory.
- If Screenshot app's translation differs from Gala's.
The 2nd one could be resolved by updating the Screenshot app to reuse Gala translations similar to how it's done here. The 1st case, on the other hand, would be tricky to support. For example user might want to save screenshots into their home directory. It would be wrong to clean up all images from their home dir because of this.
What do you think? Is this implementation good enough?
|
@Antolius can you revert changes to translation files here please? Translation files are automatically kept up to date by CI |
Remove unnecessary GLib prefixes in Housekeeping.vala in accordance with elementary code style guide.
Extend gschema schema with flag for screenshot cleanup. When enabled, include the ~/Pictures/Screenshots directory in tmpfiles configuration file. Refactor code of Housekeeping class to make it easier to add more cleanup options in the future.
Antolius
force-pushed
the
screenshots-housekeeping
branch
from
5d35079
to
bf083ca
Compare
Using gala's gettext domain to get the accurate name of Screenshots directory.
I removed that commit. |
Resolve #55
This pull request can be tested on its own by manually changing gsettings, triggering cleanup (and potentially updating system date to simulate the time passing). Cleanup can be manually triggered with:
The tmpfiles config which this feature updates can be found at
~/.config/user-tmpfiles.d/io.elementary.settings-daemon.downloads-folder.conf.