swappy icon indicating copy to clipboard operation
swappy copied to clipboard

Feature: Save as dialog

Open Maaxxs opened this issue 2 years ago • 12 comments

As mentioned in issue #125 it would be nice to save images to a different location than the default. I thought so, too and came up with the following solution. I tried to keep everything clean and simple.

  • Add a button next to the Save button with the "official" save_as image.
  • this creates a FileChooserDialog.
  • the suggested location and filename in the dialog uses the folder and file format specified in the configuration. In order to make this possible I put the part which generates the formatted filename in its own function to reuse it.

Todo:

  • [x] I'd like to add the shortcut ctrl+shift+s for the save_as dialog. I think that'd be useful, too.

Maaxxs avatar Jul 30 '22 12:07 Maaxxs

thanks for your suggestions! The _ is now replaced with a - in the .glade file as you suggested.

Other points:

  1. commits are squashed into one.
  2. I ran clang-format. Totally forgot that. Looks much better now :+1:

todo:

  • I set the title of the chooser dialog to null since I think it's not displayed actually. Do you agree?
  • translation for cancel and save text? I think this requires some more setup for the C files?

Maaxxs avatar Aug 01 '22 13:08 Maaxxs

I had some time again, so I continued here. so _("_Cancel") and similar need to be translated as usual, afaiu. (example in use in thunar with the translation)

I added some code for the initialization of textdomain and similar. I just realized that I can use the variable GETTEXT_PACKAGE from src/po/meson.build. I take another look at this tomorrow. But I'm working on this! :)

The initial step to include the translation for the dialog buttons is done.

Maaxxs avatar Dec 04 '22 22:12 Maaxxs

  // set locales according to environment variables
  setlocale(LC_ALL, "");

The default of LC_ALL is C. How the new value is set is well described in the manpage. quote:

If locale is an empty string, "", each part of the locale that should be modified is set according to the environment variables. The details are implementation-dependent. For glibc, first (regardless of catego- ry), the environment variable LC_ALL is inspected, next the environment variable with the same name as the category (see the table above), and finally the environment variable LANG. The first existing environment variable is used. If its value is not a valid locale specification, the locale is unchanged, and setlocale() returns NULL.

The locale "C" or "POSIX" is a portable locale; it exists on all con- forming systems.

I think it would be nice to do this, although it's not necessary as swappy is not using time, number units or similar where the formatting depends on the locale. AFAIU, this would enable an user of swappy to just set LC_ALL=de_DE.UTF-8 and all locales would be set to this value such as time, units, language ... Example:

$ LC_ALL=de_DE swappy -f ~/Pictures/swappytest.png
$ LANG=de_DE swappy -f ~/Pictures/swappytest.png
$ LC_ALL=en_US LANG=de_DE swappy -f ~/Pictures/swappytest.png
$ LC_ALL=en_US LANG=de_DE LANGUAGE=fr_FR swappy -f ~/Pictures/swappytest.png

The first two commands set the language to German. The third one sets it to English because (according to the manpage)

The first existing environment variable is used.

However, if LANGUAGE is specified, it's always used. Hence, the language with the last command is French.

I'm happy to remove this if you don't agree.


  // set base directory for translated messages
  bindtextdomain(GETTEXT_PACKAGE, LOCALEDIR);

This is the most important call that caused me some headache, too. The default prefix on my archlinux system is /usr/local/. When I built and installed swappy, it would look for translations in /usr/share/locale and not in /usr/local/share/locale. With this call the locale directory is set to the correct one based on the prefix, e.g. /usr/ or /tmp/testroot.


  // explicitly set encoding of message translations to UTF-8
  bind_textdomain_codeset(GETTEXT_PACKAGE, "UTF-8");

this forces the codeset to UTF-8. I think this is ok as the translation files are all UTF-8 as well. For instance, if we don't do this, then I get the following warnings if only de_DE instead of de_DE.UTF-8 is specified.

$ LC_ALL=de_DE swappy -f ~/Pictures/swappytest.png

(swappy:179132): Pango-WARNING **: 14:07:42.612: Invalid UTF-8 string passed to pango_layout_set_text()
(swappy:179132): Pango-WARNING **: 14:07:42.624: Invalid UTF-8 string passed to pango_layout_set_text()

btw, should I continue to force push changes in that one commit or squash the commits in the end? (I always feel kinda bad when doing a force push :) ).

Maaxxs avatar Dec 05 '22 13:12 Maaxxs

Thank you @Maaxxs. I'll have a look when I get some time.

You can definitely keep "force pushing", it's your branch on your own fork you can do whatever you want with it.

jtheoof avatar Dec 07 '22 16:12 jtheoof

Thank you @Maaxxs. I'll have a look when I get some time.

Sure. Thanks!

You can definitely keep "force pushing", it's your branch on your own fork you can do whatever you want with it.

Ok. My thinking was that it may be easier for you to review changes. And I think GitHub get confused sometimes when you do a force push and there are open reviews. So if you don't mind, I'll do normal commits and squash them in the end so that only one commit gets merged.

Maaxxs avatar Dec 09 '22 14:12 Maaxxs

hi @jtheoof have you had time for another look at this?

Maaxxs avatar Aug 03 '23 10:08 Maaxxs

nice to see some activity here. Not sure what I can do to push this further.

I'm running with these changes since I proposed them and so far (~2 years :) ) it works well.

Maaxxs avatar Apr 23 '24 05:04 Maaxxs