feat(config): add quit_on_copy config #134

SeppeSoete · 2022-07-31T10:08:38Z

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

jtheoof · 2022-07-31T21:19:25Z

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

The CI won't pass. The commit is not following the README instructions on convetionnal commits.
The added config is missing documentation in the README.md and swappy.1.scd.
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 · 2022-07-31T21:49:27Z

@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 · 2022-07-31T21:59:03Z

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

jtheoof · 2022-08-01T00:41:13Z

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.

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

What do you think?

SeppeSoete · 2022-08-01T15:14:46Z

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.

2. Add the exit logic upon Save action (including ctrl+s shortcut).

done & tested

3. Update variables in the MR according to choice in 1.

and done

jtheoof · 2022-08-02T02:44:40Z

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

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

README.md

src/application.c

src/clipboard.c

SeppeSoete · 2022-08-02T15:34:44Z

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

Almost there.

jtheoof · 2022-08-03T00:24:48Z

README.md

@@ -49,6 +49,7 @@ line_size=5
 text_size=20
 text_font=sans-serif
 paint_mode=brush
+quit_after_export=false


Suggested change

quit_after_export=false

early_exit=false

jtheoof · 2022-08-03T00:31:26Z

src/application.c

+  config_free(state);
+}
+
+static void early_exit(struct swappy_state *state){


You don't need this function. It's actually a bit misleading from the function calling this because it doesn't always exit the app.

Calling gtk_main_quit will exit the main loop and call application_finish in main.c

jtheoof · 2022-08-03T00:33:06Z

src/application.c

@@ -191,6 +221,8 @@ static void save_state_to_file_or_folder(struct swappy_state *state,
  }

  g_object_unref(pixbuf);
+
+  early_exit(state);


Suggested change

early_exit(state);

if (state->config->early_exit) {

gtk_main_quit();

}

src/application.c

jtheoof · 2022-08-03T00:35:00Z

src/application.c

@@ -290,6 +300,7 @@ 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);


Put the same logic inside clipboard_copy_drawing_area_to_selection.

i.e:

Suggested change

clipboard_copy_drawing_area_to_selection(state);

if (state->config->early_exit) {

gtk_main_quit();

}

jtheoof · 2022-08-03T00:36:07Z

src/application.c

@@ -319,6 +330,7 @@ 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);


better to put the logic inside clipboard_copy_drawing_area_to_selection

jtheoof · 2022-08-03T00:57:31Z

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 · 2022-08-03T00:58:39Z

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 · 2022-08-03T16:02:46Z

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 · 2022-08-03T23:58:12Z

Thanks for your contribution !

SeppeSoete changed the title ~~Added a configuration option to quit the application on copy~~ Feature: configuration option to quit the application on copy Jul 31, 2022

SeppeSoete changed the title ~~Feature: configuration option to quit the application on copy~~ feat(config): add quit_on_copy config Jul 31, 2022

SeppeSoete force-pushed the feature/close_on_copy branch from 39f8f7d to 602be09 Compare July 31, 2022 21:37

SeppeSoete force-pushed the feature/close_on_copy branch from cce9aad to 2c1187d Compare August 1, 2022 15:08

jtheoof requested changes Aug 2, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

src/application.c Outdated Show resolved Hide resolved

src/clipboard.c Outdated Show resolved Hide resolved

SeppeSoete requested a review from jtheoof August 2, 2022 15:32

jtheoof requested changes Aug 3, 2022

View reviewed changes

feat(config): add early_exit option

27d611f

SeppeSoete force-pushed the feature/close_on_copy branch from ad3ce5b to 27d611f Compare August 3, 2022 16:00

jtheoof merged commit 60da549 into jtheoof:master Aug 3, 2022

AdjectiveAllison mentioned this pull request Feb 4, 2024

[Feature request] Autosave on close #107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(config): add quit_on_copy config #134

feat(config): add quit_on_copy config #134

SeppeSoete commented Jul 31, 2022

jtheoof commented Jul 31, 2022

SeppeSoete commented Jul 31, 2022

jtheoof commented Jul 31, 2022

jtheoof commented Aug 1, 2022

SeppeSoete commented Aug 1, 2022

jtheoof commented Aug 2, 2022

SeppeSoete commented Aug 2, 2022

jtheoof left a comment

jtheoof Aug 3, 2022

jtheoof Aug 3, 2022

jtheoof Aug 3, 2022

jtheoof Aug 3, 2022

jtheoof Aug 3, 2022

jtheoof Aug 3, 2022

jtheoof commented Aug 3, 2022

jtheoof commented Aug 3, 2022

SeppeSoete commented Aug 3, 2022

jtheoof commented Aug 3, 2022

feat(config): add quit_on_copy config #134

feat(config): add quit_on_copy config #134

Conversation

SeppeSoete commented Jul 31, 2022

jtheoof commented Jul 31, 2022

SeppeSoete commented Jul 31, 2022

jtheoof commented Jul 31, 2022

jtheoof commented Aug 1, 2022

SeppeSoete commented Aug 1, 2022

jtheoof commented Aug 2, 2022

SeppeSoete commented Aug 2, 2022

jtheoof left a comment

Choose a reason for hiding this comment

jtheoof Aug 3, 2022

Choose a reason for hiding this comment

jtheoof Aug 3, 2022

Choose a reason for hiding this comment

jtheoof Aug 3, 2022

Choose a reason for hiding this comment

jtheoof Aug 3, 2022

Choose a reason for hiding this comment

jtheoof Aug 3, 2022

Choose a reason for hiding this comment

jtheoof Aug 3, 2022

Choose a reason for hiding this comment

jtheoof commented Aug 3, 2022

jtheoof commented Aug 3, 2022

SeppeSoete commented Aug 3, 2022

jtheoof commented Aug 3, 2022