jtheoof / swappy

A Wayland native snapshot editing tool, inspired by Snappy on macOS
MIT License
1k stars 40 forks source link

feat(config): add quit_on_copy config #134

Closed SeppeSoete closed 2 years ago

SeppeSoete commented 2 years ago

if the quit_on_copy config option is set to true, the application now exits after copying the selection to the keyboard

jtheoof commented 2 years ago

Thanks for your contribution @SeppeSoete. A few things to consider:

  1. The CI won't pass. The commit is not following the README instructions on convetionnal commits.
  2. The added config is missing documentation in the README.md and swappy.1.scd.
  3. Even if those 2 points were fixed. I don't know if I would merge this. I find the behaviour counter intuitive, even if it's opt-in. I'd wait to see if others are interested in this behaviour.
SeppeSoete commented 2 years ago

@jtheoof Thanks for your response! I've made some changes in response to your comment.

1. The CI won't pass. The commit is not following the README instructions on convetionnal commits.

I have now changed the commit message and as far as I know it should now conform to the conventional commits standard.

2. The added config is missing documentation in the `README.md` and `swappy.1.scd`.

Good catch, those have been added now.

3. Even if those 2 points were fixed. I don't know if I would merge this. I find the behaviour counter intuitive, even if it's opt-in. I'd wait to see if others are interested in this behaviour.

The reason I wanted this behaviour is because it is the default behaviour for at least some screenshot utilities like for example flameshot, which is the utility I used before making the switch to wayland. For me it was actually counter intuitive that the application remained running after copying to the clipboard and I thought the application was bugged until I noticed it had in fact copied the image to my clipboard.

jtheoof commented 2 years ago

All right let me think about it and try out flameshot. Thanks for the quick feedback.

jtheoof commented 2 years ago

After putting more thoughts into it, I'm not against the idea, but I think it should a go a step further. Making it close swappy on save as well.

  1. Rename the option to quit_on_main_action or quit_or_copy_and_save or something better if you find a better idea.
  2. Add the exit logic upon Save action (including ctrl+s shortcut).
  3. Update variables in the MR according to choice in 1.

What do you think?

SeppeSoete commented 2 years ago
1. Rename the option to `quit_on_main_action` or `quit_or_copy_and_save` or something better if you find a better idea.

I ended up calling it quit_after_export since I felt like that was most representative.

  1. Add the exit logic upon Save action (including ctrl+s shortcut). done & tested
  2. Update variables in the MR according to choice in 1. and done
jtheoof commented 2 years ago

Thanks for the update. I've still got some comments:

  1. quit_after_export is not really a good name. A copy is not exporting anything. Let's settle on early_exit (Sorry for the back and fourth, naming is important once people have their config files setup. We don't want to break this afterwards).
  2. The application is exiting a bit to harshly with the exit(0). The application_finish function should be called in order to release the memory. There must be a more gtk friendly way to handle that.
SeppeSoete commented 2 years ago

The early_exit function should gracefully exit now after freeing everything, I did need to move the application_finish function up higher because of implicit declaration errors though.

jtheoof commented 2 years ago
From 30c04cf08bc2be2b4e949fcef68d979614aefe28 Mon Sep 17 00:00:00 2001
From: Jeremy Attali <contact@jtheoof.me>
Date: Tue, 2 Aug 2022 20:48:48 -0400
Subject: [PATCH] feat(config): fix early_exit

---
 src/application.c | 15 ++++-----------
 src/clipboard.c   |  4 ++++
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/src/application.c b/src/application.c
index 86d35e8..8a6c2f7 100644
--- a/src/application.c
+++ b/src/application.c
@@ -46,6 +46,7 @@ static void update_ui_panel_toggle_button(struct swappy_state *state) {
 }

 void application_finish(struct swappy_state *state) {
+  g_debug("application finishing, cleaning up");
   paint_free_all(state);
   pixbuf_free(state);
   cairo_surface_destroy(state->rendering_surface);
@@ -67,14 +68,6 @@ void application_finish(struct swappy_state *state) {
   config_free(state);
 }

-static void early_exit(struct swappy_state *state){
-  // Exit the application if early_exit configuration is enabled
-  if (state->config->early_exit){
-    application_finish(state);
-    gtk_main_quit();
-  }
-}
-
 static void action_undo(struct swappy_state *state) {
   GList *first = state->paints;

@@ -222,7 +215,9 @@ static void save_state_to_file_or_folder(struct swappy_state *state,

   g_object_unref(pixbuf);

-  early_exit(state);
+  if (state->config->early_exit) {
+    gtk_main_quit();
+  }
 }

 static void maybe_save_output_file(struct swappy_state *state) {
@@ -300,7 +295,6 @@ void copy_clicked_handler(GtkWidget *widget, struct swappy_state *state) {
   // Commit a potential paint (e.g. text being written)
   commit_state(state);
   clipboard_copy_drawing_area_to_selection(state);
-  early_exit(state);
 }

 void control_modifier_changed(bool pressed, struct swappy_state *state) {
@@ -330,7 +324,6 @@ void window_keypress_handler(GtkWidget *widget, GdkEventKey *event,
     switch (event->keyval) {
       case GDK_KEY_c:
         clipboard_copy_drawing_area_to_selection(state);
-        early_exit(state);
         break;
       case GDK_KEY_s:
         save_state_to_file_or_folder(state, NULL);
diff --git a/src/clipboard.c b/src/clipboard.c
index 188c33c..dd03dd9 100644
--- a/src/clipboard.c
+++ b/src/clipboard.c
@@ -82,5 +82,9 @@ bool clipboard_copy_drawing_area_to_selection(struct swappy_state *state) {

   g_object_unref(pixbuf);

+  if (state->config->early_exit) {
+    gtk_main_quit();
+  }
+
   return true;
 }
-- 
2.37.1
jtheoof commented 2 years ago

Please apply the above patch and squash all commit into one: feat(config): add early_exit option and make sure clang-format has been run.

Thanks.

SeppeSoete commented 2 years ago

I ran clang-format -i on all the files in /include/ and /src/, no changes made though but that was after applying your patch so the format issue was already gone.

jtheoof commented 2 years ago

Thanks for your contribution !