Feature: Save as dialog #133

Maaxxs · 2022-07-30T12:49:59Z

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:

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

jtheoof

Thanks for your contribution @Maaxxs. I'm not against it but there are few things to change.

Can you squash all your commits into 1?
Make sure clang-format is run on all the files you're changing.
Please have a look at my comments and suggestions.

res/swappy.glade

src/application.c

Maaxxs · 2022-08-01T13:55:18Z

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

Other points:

commits are squashed into one.
I ran clang-format. Totally forgot that. Looks much better now 👍

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?

jtheoof · 2022-08-02T03:01:24Z

src/application.c

+  filename_suggestion = format_filename(state->config->save_filename_format);
+
+  dialog = gtk_file_chooser_dialog_new(NULL, state->ui->window, action,
+                                       ("_Cancel"), GTK_RESPONSE_CANCEL,


Is there a reason why you're putting the "_Cancel" and "_Save" between parenthesis?

Regarding translating those, I'm not sure. The gtk doc seems to indicate what you are doing is valid. But I don't know if it means it's properly translated and using the base gtk po files. I'd need to research.

I've switched to French, and the Close / Save do not seem to be translated. You'll have to use gettext I guess? I don't know if the ninja target will be smart enough to catch those. (I don't think there any translation done in the .c files. It's only in the ui files for now.)

Good luck. :)

Maaxxs · 2022-12-04T22:49:27Z

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.

Allow to save snapshots with a different name and under a different location than specified in the configuration as default. Signed-off-by: Max Kunzelmann <[email protected]>

Maaxxs · 2022-12-05T13:57:42Z

  // 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 :) ).

jtheoof · 2022-12-07T16:13:55Z

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.

Maaxxs · 2022-12-09T14:54:58Z

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 · 2023-08-03T10:44:52Z

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

Maaxxs · 2024-04-23T05:54:24Z

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.

leon-erd · 2024-05-09T13:23:49Z

Would be really nice to see indeed!

rubiin · 2024-05-09T15:24:57Z

I am not sure whats causing the delay either, lets wait for the review

gebeer · 2024-05-24T08:19:54Z

I'd really love to have this feature, too. Would make swappy an even greater replacement for flameshot.

rubiin · 2024-05-24T09:29:48Z

@Maaxxs can you ask for review once more.
cc @jtheoof

enzi · 2024-07-04T13:51:46Z

I just looked up if Snappy has this feature.
Doesn't look like @jtheoof will merge anytime soon, so I compiled your fork. Thanks @Maaxxs !

jtheoof · 2024-10-26T00:24:51Z

I'm still hesitating to merge this. I think the part that I'm not a fan of is the fact that it clutters the UI. I want this tool to be simple and we're adding an extra button. I would be down to merge if you remove the UI bits and just use the ctrl+shift+s shortcut + document it properly in the README and the manpage

auroraanna · 2024-10-27T17:05:32Z

are there other functions that are only accessible by keyboard shortcut?

kadamski · 2024-11-08T08:53:49Z

src/pixbuf.c

+  char *filename;
+  filename = format_filename(filename_format);
+  if (filename == NULL) return;
+
  g_snprintf(path, MAX_PATH, "%s/%s", folder, filename);


I think in this case we actually should abort if this fails. We do not want to write to a truncated filename as this could potentially overwrite some file the user did not intend to overwrite.

thanks for taking a look at the code!
I hope I get around to taking another look at this at the weekend but I have lots of other things on my mind currently.

kadamski · 2024-11-08T09:03:39Z

@jtheoof you have the authority here but if I may add my 0.02$ here then I do not think this change is cluttering the UI too much. It just adds one small button in the area that has plenty of space anyway. I would like to have that functionality merged, though, so if you still feel you don't want the UI then I think having just the key shortcut is better than nothing so if @Maaxxs wont do this I would modify his PR to suit your request. But maybe there's still a chance you could change your mind? :)

Maaxxs · 2024-11-11T09:12:38Z

I'm still hesitating to merge this. I think the part that I'm not a fan of is the fact that it clutters the UI. I want this tool to be simple and we're adding an extra button.

I understand. I'm a huge fan of simple and clean tools. However, I don't think that adding this button clutters the UI.
I think the button is useful insofar that it adds the easily discoverable feature to save a screenshot at a non-default location.

I would be down to merge if you remove the UI bits and just use the ctrl+shift+s shortcut + document it properly in the README and the manpage

I'd probably do this if you were strictly against adding the button. As was previously said it's better than not having this feature at all. I use it all the time when taking screenshots for different projects meaning that the directory I'm saving the screenshot to changes every time.

jtheoof requested changes Jul 31, 2022

View reviewed changes

res/swappy.glade Outdated Show resolved Hide resolved

res/swappy.glade Outdated Show resolved Hide resolved

res/swappy.glade Outdated Show resolved Hide resolved

res/swappy.glade Show resolved Hide resolved

src/application.c Outdated Show resolved Hide resolved

Maaxxs force-pushed the feat/save_as_image branch 2 times, most recently from 2dc2642 to cc173c2 Compare August 1, 2022 13:48

Maaxxs force-pushed the feat/save_as_image branch from cc173c2 to 07e9e45 Compare August 1, 2022 20:38

jtheoof reviewed Aug 2, 2022

View reviewed changes

jtheoof force-pushed the master branch 2 times, most recently from 1c3eaa2 to 3083bb7 Compare November 20, 2022 21:48

Maaxxs force-pushed the feat/save_as_image branch from 07e9e45 to 18a899a Compare December 4, 2022 22:26

feat(save_as): add save_as button for snapshots

e670f72

Allow to save snapshots with a different name and under a different location than specified in the configuration as default. Signed-off-by: Max Kunzelmann <[email protected]>

Maaxxs force-pushed the feat/save_as_image branch from 18a899a to e670f72 Compare December 5, 2022 13:54

Maaxxs mentioned this pull request Aug 3, 2023

Ask where to save files. #125

Open

rubiin approved these changes Apr 22, 2024

View reviewed changes

kadamski reviewed Nov 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Save as dialog #133

Feature: Save as dialog #133

Maaxxs commented Jul 30, 2022 •

edited

Loading

jtheoof left a comment

Maaxxs commented Aug 1, 2022

jtheoof Aug 2, 2022

jtheoof Aug 2, 2022

Maaxxs commented Dec 4, 2022

Maaxxs commented Dec 5, 2022 •

edited

Loading

jtheoof commented Dec 7, 2022

Maaxxs commented Dec 9, 2022

Maaxxs commented Aug 3, 2023

Maaxxs commented Apr 23, 2024

leon-erd commented May 9, 2024

rubiin commented May 9, 2024 •

edited

Loading

gebeer commented May 24, 2024

rubiin commented May 24, 2024

enzi commented Jul 4, 2024

jtheoof commented Oct 26, 2024

auroraanna commented Oct 27, 2024

kadamski Nov 8, 2024

Maaxxs Nov 11, 2024

kadamski commented Nov 8, 2024

Maaxxs commented Nov 11, 2024

Feature: Save as dialog #133

Are you sure you want to change the base?

Feature: Save as dialog #133

Conversation

Maaxxs commented Jul 30, 2022 • edited Loading

jtheoof left a comment

Choose a reason for hiding this comment

Maaxxs commented Aug 1, 2022

jtheoof Aug 2, 2022

Choose a reason for hiding this comment

jtheoof Aug 2, 2022

Choose a reason for hiding this comment

Maaxxs commented Dec 4, 2022

Maaxxs commented Dec 5, 2022 • edited Loading

jtheoof commented Dec 7, 2022

Maaxxs commented Dec 9, 2022

Maaxxs commented Aug 3, 2023

Maaxxs commented Apr 23, 2024

leon-erd commented May 9, 2024

rubiin commented May 9, 2024 • edited Loading

gebeer commented May 24, 2024

rubiin commented May 24, 2024

enzi commented Jul 4, 2024

jtheoof commented Oct 26, 2024

auroraanna commented Oct 27, 2024

kadamski Nov 8, 2024

Choose a reason for hiding this comment

Maaxxs Nov 11, 2024

Choose a reason for hiding this comment

kadamski commented Nov 8, 2024

Maaxxs commented Nov 11, 2024

Maaxxs commented Jul 30, 2022 •

edited

Loading

Maaxxs commented Dec 5, 2022 •

edited

Loading

rubiin commented May 9, 2024 •

edited

Loading