Merge ~ubuntu-oi/xpad:patch_#2084343 into xpad:master

[Emmanuel]

Solve issue #2084343

xpad don't quit when all notes are hidden
xpad -s can be used to show all notes on start
xpad -h can be used to hide all notes on start

CLI command are now working for the active instance
xpad -s : show all notes
xpad -h : hide all notes
xpad -t : toggle individual hide/show status for each notes (was before hide all or show all)
xpad -q : quit xpad

070ba52... by SwingRock <email address hidden> on 2025-01-11

patch #2084343

[Arthur Borsboom (arthurborsboom)]

I have ran a couple of the simple tests with the patch.
Nothing broke, so that is good. :-)

A couple of nits:

* Remove xpad_app_first_idle_check, since it does nothing anymore; it always returns false.
* Use lowercase for variable names - GSList *Pad - coding convention as far as I know.

Once done, I will merge it into master.
Thanks for helping out.

review: Needs Fixing

[Emmanuel (ubuntu-oi)]

I had a couple of question.
* where can i find coding convention ?
* i keep xpad_app_first_idle_check for now because i've some question :
    - when xpad is launched and there's no visible notes and no icon tray, should we do nothing or display a note with small help focused on explaining Command line only
* i change the way "-t" works. is it fine ?
    - before "xpad -t" : toggle global status hide/show. One hidden : it show all. all visible : it hides all.
    - after "xpad -t" : toggle individual notes status hide/show
    - perhaps having the 2 possibility would be the best. Tell me.
* As consequences, i change the way xpad_pad_group_toggle_hide works
    - i rename it xpad_pad_group_toggle_hide_show
    - but is now for individual notes not for all notes
    - The question is : should we keep the fonction in xpad-pad-group.c or place it in xpad-pad.c ?
* naming convention for CLI option
    - is it ok for you if lowercase are for action to all pads and uppercase for individuals notes ?
    - if ok, i would like to intorduce some CLI option. to discuss later.
* last one : i still don't know what to do when using combination like :
    - "-nh" : create new one and hide all pads, so we don't see the new one ?
    - "-hn" : hide all pads and create new one (which is visible) !!
Waiting for your answer before changing code
I'm glade to help, cause i really like xpad.

[Arthur Borsboom (arthurborsboom)]

> where can i find coding convention ?

I use coding conventions as a guideline, not as a strict rule. But in most languages in which I code, variable names start with lowercase, while class names and constants start with uppercase.

First hit in Google gave me this C coding convention.

https://www.gnu.org/prep/standards/html_node/Writing-C.html
https://www.gnu.org/prep/standards/html_node/Names.html#Names

Possibly GTK coding conventions might be more appropriate.

> when xpad is launched and there's no visible notes and no icon tray, should we do nothing or display a note with small help focused on explaining Command line only

I think do nothing is better. If not, every time where Xpad is started it would show that help or note. Besides there is already a first time/new user help.

All the other questions, I need to give more thought than this quick answer. :-)

But realize that if the behavior changes of a CLI command, that this might break existing scripts.

[Arthur Borsboom (arthurborsboom)]

Your questions are all valid and give material to think about.
Honestly I don't have all the answers, but maybe we can find working solutions.

> i change the way "-t" works. is it fine ?

I prefer not to change existing behavior, since this might break this app for users who depend on this behavior. Of course, when a certain functionality is broken, we have to rethink the best approach.

If new/different functionality is needed, I prefer it is added somehow. You suggested adding several command line options. Also, to keep the app as usefull as possible, we might want to have a sneak peak on conventions for command line arguments as well. I haven't ever looked at it before, but it might be a good idea to learn from other people's mistakes by using best practices.

I found these conventions, but if you have other conventions, that is probably fine as well.

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html

If all of the above gets a bit overwhelming, we can also split all the changes in smaller changes and handle one by one.

What do you think?
Suggestions?

[Emmanuel (ubuntu-oi)]

Great answer.
Thanks a lot
here what i think must be done :
- get "-t" to its previous function : toggle all show/hide
- introduce a new command : probably -T : toggle individuals notes show/hide
- read those guide carefully. It's not the way i usually do.
- do nothing about "-nh" and "-hn", we will see later if someone is using it and complain about the way is it.

I'll publish changes Wednesday night (France hour).
No suggestion at right now. I need to think about and read the convention you send to me

Regards,

[Arthur Borsboom (arthurborsboom)]

Nice. I like your plan.

To make patches easy to review and/or rollback, please keep them simple and split them if they change different/unrelated parts.

Enjoy coding; I'll help, review and support where I can. :-)

b7e1040... by SwingRock <email address hidden> on 2025-01-15

patch #2084343 V2

[Emmanuel (ubuntu-oi)]

Ok, ready for review.
Changes are :
doc/xpad.1 :
    - man update with new -T
src/xpad-app.c :
    - Remove : xpad_app_first_idle_check
    - So remove the call to g_idle_add ((GSourceFunc)xpad_app_first_idle_check, pad_group);
    - Adding variable for new -T command
    - Renaming option_toggle to option_toggle_all (for clarity purpose)
src/xpad-pad-group.c :
    - adding function xpad_pad_group_toggle_hide_show to toggle individual pads
    - old function xpad_pad_group_toggle_hide was restored
src/xpad-pad.c :
    - removing another case that quit xpad if no indicator tray and no note visible.

We also need to ask Andrew if it's working fine for him.

Regards

[Arthur Borsboom (arthurborsboom)]

I have reviewed the merge request resulting in the following questions and suggestions.

You rename 'Toggle between show and hide all pads' > 'Toggle all pads between show and hide' and rename the variable from option_toggle > option_toggle_all. This seems like a good improvement.

Would you mind to put this in a single patch, so I can merge this while discussing the remaining topics?

You add an option 'Toggle each pad between show and hide' including a new flag option_toggle_pads. By reading only this part, it is not clear what the difference is between option_toggle_all and option_toggle_pads.

Reading the actual function, I see that the pads which are hidden are shown and the ones that are shown are hidden. I am not yet seeing the use for this. I thought the use case would be to be able to toggle (show/hide) a single pad, where the challenge is to provide the UID on the CLI.

Can you give an example for the 'invert all pads' use case ?

To come back to splitting the patches, I see three topics in the merge request.

- Rename toggle > toggle all pads
- Add new feature 'inverse all pads'
- Prevent Xpad to quit in case of no (visible) pads

It would be helpful, to split the patches by topic, so if we agree on a topic, I can merge this part. It is also easier to review and to rollback.

Typo in comment: Toogle

Is this helpful ?

[Emmanuel (ubuntu-oi)]

you are right, there's 4 modifications.
1st - just some renaming of the existing function that toggle notes hide/show this way : If one is hidden, then Show all and if all are visible then hide all. I do this for clarity purpose.

2nd - i add a new function (and a new CLI option -T ) which allow to toggle notes show/hide individually. for each note if it's hidden it show and if visible it hides.

I do not understand why xpad should quit if no notes are visible
nor why xpad should show all notes if they are all hidden and no xpad in notification tray.
so i change the following :

3rd is to avoid xpad to quit when all pads are hidden AND xpad not in notification tray.
4th I removes the code that does : "if xpad is launched and there's no visible notes, it shows all notes". So with the patch : if in the save directory all notes are hidden then when xpad is started, notes stay hidden and xpad do not quit.

3. and 4. are required to solve the issue #2084343.
1. and 2. are improvement and could be separated

Tell me what you would like

[Arthur Borsboom (arthurborsboom)]

I prefer 4 separate patches. :-)

The thing I don't understand yet is how the new functionality (which I
call) 'toggle_invert_pads' solves the issue.
Andrew and other user are looking for a CLI command that can show or
hide a specific pad.
The new functionality applies to all pads, which in essence inverts
the show/hide status.

As far as I can see, I could apply 3 out 4 patches and we need to
rethink the new functionality.

What do you think?

On Mon, 27 Jan 2025 at 20:35, Emmanuel <email address hidden> wrote:
>
> you are right, there's 4 modifications.
> 1st - just some renaming of the existing function that toggle notes hide/show this way : If one is hidden, then Show all and if all are visible then hide all. I do this for clarity purpose.
>
> 2nd - i add a new function (and a new CLI option -T ) which allow to toggle notes show/hide individually. for each note if it's hidden it show and if visible it hides.
>
> I do not understand why xpad should quit if no notes are visible
> nor why xpad should show all notes if they are all hidden and no xpad in notification tray.
> so i change the following :
>
> 3rd is to avoid xpad to quit when all pads are hidden AND xpad not in notification tray.
> 4th I removes the code that does : "if xpad is launched and there's no visible notes, it shows all notes". So with the patch : if in the save directory all notes are hidden then when xpad is started, notes stay hidden and xpad do not quit.
>
> 3. and 4. are required to solve the issue #2084343.
> 1. and 2. are improvement and could be separated
>
> Tell me what you would like
>
> --
> https://code.launchpad.net/~ubuntu-oi/xpad/+git/xpad/+merge/479250
> You are reviewing the proposed merge of ~ubuntu-oi/xpad:patch_#2084343 into xpad:master.
>

[Andrew Myers (andym48)]

Sorry to but in but:

> Andrew and other user are looking for a CLI command that can show or
hide a specific pad.

In fact I had probably assumed that -t would toggle hide/display the selected notes. So if we have 10 notes and 3 are displayed -t would hide the 3 notes and a further -t would display the 3 notes.

On the right-click notes menu we already have: Notes > Show All

[Emmanuel (ubuntu-oi)]

Hi, i'll try to clarify.
Andrew is fine with the current CLI option. No need to change or add anything.
the strange behavior he got are coming from the fact that he is in an environnement without notification tray and so "xpad exit when all notes are hidden" AND "when started xpad show all notes if all loaded pads are hidden"
In order to sole his issue we need to :
- avoid xpad to quit when all pads are hidden.
- remove the code that display all notes at startup if all loaded pads have a hidden status.
That's all for Andrew. this could be the patch #1

patch #2 : explain better CLI option -t and rename function. It's the way it works.
xpad -t does the following :
- If one notes is hidden then all notes show.
- If all notes are visible, then all notes hide.
This is a global show hide status change because at the end all notes got the same status.

patch #3 : add CLI option -T
- it toggle individual show/hide status
so notes hidden becomes visible and visible notes becomes hidden

What you suggest is another improvement : it will require 3 new CLI option :
* -l to list all notes with a number
* -S# to show note number #
* -H# to hide note number #
* -T# to toggle show/hide status of note number #
But for now nobody ask for it.

So the current merge could be patch #1
and i could create 2 others for patch #2 and #3
also create an enhancement request to add new CLI options (with low priority)

[Emmanuel (ubuntu-oi)]

When you say : "In fact I had probably assumed that -t would toggle hide/display the selected notes. So if we have 10 notes and 3 are displayed -t would hide the 3 notes and a further -t would display the 3 notes."

This is wrong.
If we have 10 notes and 3 are displayed
xpad -t will display all notes
a further xpad -t will hide all notes

That's the way it works on 5.8.0

[Andrew Myers (andym48)]

Yes, I understand that is how it works at present. I simply reported what I had assumed. Just imagine what happens at the moment if you have 100 notes ;). If it is a lot of work then ignore me.

For me, these are not relevant:

>What you suggest is another improvement : it will require 3 new CLI option :
>* -l to list all notes with a number
>* -S# to show note number #
>* -H# to hide note number #
>* -T# to toggle show/hide status of note number #
>But for now nobody ask for it.

referring to note numbers (#) would have little value since the user would not normally be aware of the number relating to a specific note. The existing menu options are sufficient I think.

[Emmanuel (ubuntu-oi)]

It will took me some times to separate patch #1, #2 and #3.
I understand your request but i'm not motivated to do it,
so i'll do it when i'll be in a good mood.
Most probably during the holidays in February.

[Arthur Borsboom (arthurborsboom)]

I like your honesty. No problem. :-)

On Sat, 1 Feb 2025 at 08:59, Emmanuel <email address hidden> wrote:
>
> It will took me some times to separate patch #1, #2 and #3.
> I understand your request but i'm not motivated to do it,
> so i'll do it when i'll be in a good mood.
> Most probably during the holidays in February.
> --
> https://code.launchpad.net/~ubuntu-oi/xpad/+git/xpad/+merge/479250
> You are reviewing the proposed merge of ~ubuntu-oi/xpad:patch_#2084343 into xpad:master.
>

Unmerged commits

b7e1040... by SwingRock <email address hidden> on 2025-01-15

patch #2084343 V2

070ba52... by SwingRock <email address hidden> on 2025-01-11

patch #2084343

diff --git a/doc/xpad.1 b/doc/xpad.1
index 42c0d7f..c4f146a 100644
--- a/doc/xpad.1
+++ b/doc/xpad.1
@@ -25,7 +25,10 @@ Hides existing pads on startup.  All pads will be hidden, so unless the tray ico
 Shows existing pads on startup.
 .TP
 .B \-t, \-\-toggle
-Toggle between showing and hiding each of the existing pads on startup.
+Toggle all pads between showing and hiding. One hidden, it shows all. All visible, it hides all.
+.TP
+.B \-T, \-\-toggle-notes
+Toggle individually each pad between showing and hiding.
 .TP
 .B \-n, \-\-new
 Open a new pad.
diff --git a/src/xpad-app.c b/src/xpad-app.c
index 1bd8cf8..08ad38f 100644
--- a/src/xpad-app.c
+++ b/src/xpad-app.c
@@ -64,7 +64,8 @@ static gboolean option_nonew;
 static gboolean option_new;
 static gboolean option_hide;
 static gboolean option_show;
-static gboolean option_toggle;
+static gboolean option_toggle_all;
+static gboolean option_toggle_pads;
 static gboolean option_version;
 static gboolean option_quit;
 static gboolean shutdown_in_progress;
@@ -87,7 +88,6 @@ static gchar		*make_config_dir            (void);
 static void		register_stock_icons        (void);
 static gint		xpad_app_load_pads          (void);
 static gboolean		xpad_app_quit_if_no_pads    (XpadPadGroup *group);
-static gboolean		xpad_app_first_idle_check   (XpadPadGroup *group);
 static gboolean		xpad_app_pass_args          (void);
 static gboolean		xpad_app_open_proc_file     (void);
 static void enable_unix_signal_handlers();
@@ -195,8 +195,6 @@ xpad_app_init (int argc, char **argv)
 	xpad_periodic_set_callback ("save-content", (XpadPeriodicFunc) xpad_pad_save_content);
 	xpad_periodic_set_callback ("save-info", (XpadPeriodicFunc) xpad_pad_save_info);
 
-	g_idle_add ((GSourceFunc)xpad_app_first_idle_check, pad_group);
-
 	if (first_time) {
 		g_object_set (settings, "autostart-xpad", TRUE, NULL);
 		show_help ();
@@ -433,32 +431,6 @@ register_stock_icons (void)
 	g_free(theme_dir);
 }
 
-static gboolean
-xpad_app_first_idle_check (XpadPadGroup *group)
-{
-	/* We do this check at the first idle rather than immediately during
-	   start because we want to give the tray time to become embedded. */
-	if (!xpad_tray_has_indicator () &&
-	    xpad_pad_group_num_visible_pads (group) == 0)
-	{
-		if (pads_loaded_on_start > 0)
-			/* So we loaded xpad, there's no tray, and there's only hidden
-			   pads...  Probably previously had tray open but we failed
-			   this time.  Show all pads as a last resort.  This shouldn't
-			   happen in normal operation. */
-			xpad_pad_group_show_all (group);
-		else
-		{
-			if (gtk_main_level () > 0)
-				xpad_app_quit ();
-			else
-				exit (0);
-		}
-	}
-
-	return FALSE;
-}
-
 /* Scans config directory for pad files and loads them. */
 static gint
 xpad_app_load_pads (void)
@@ -804,7 +776,8 @@ static GOptionEntry remote_options[] =
 	{"new", 'n', 0, G_OPTION_ARG_NONE, &option_new, N_("Create a new pad on startup even if pads already exist"), NULL},
 	{"hide", 'h', 0, G_OPTION_ARG_NONE, &option_hide, N_("Hide all pads"), NULL},
 	{"show", 's', 0, G_OPTION_ARG_NONE, &option_show, N_("Show all pads"), NULL},
-	{"toggle", 't', 0, G_OPTION_ARG_NONE, &option_toggle, N_("Toggle between show and hide all pads"), NULL},
+	{"toggle", 't', 0, G_OPTION_ARG_NONE, &option_toggle_all, N_("Toggle all pads between show and hide"), NULL},
+	{"toggle-notes", 'T', 0, G_OPTION_ARG_NONE, &option_toggle_pads, N_("Toggle each pad between show and hide"), NULL},
 	{"new-from-file", 'f', 0, G_OPTION_ARG_FILENAME_ARRAY, &option_files, N_("Create a new pad with the contents of a file"), N_("FILE")},
 	{"quit", 'q', 0, G_OPTION_ARG_NONE, &option_quit, N_("Close all pads"), NULL},
 	{"sm-client-id", 0, G_OPTION_FLAG_HIDDEN, G_OPTION_ARG_STRING, &option_smid, NULL, NULL},
@@ -866,7 +839,9 @@ process_remote_args (gint *argc, gchar **argv[], gboolean have_gtk, XpadSettings
 	option_smid = NULL;
 	option_hide = FALSE;
 	option_show = FALSE;
-	option_toggle = FALSE;
+	option_toggle_all = FALSE;
+	option_toggle_pads = FALSE;
+
 
 	context = g_option_context_new (NULL);
 	g_option_context_set_ignore_unknown_options (context, TRUE);
@@ -895,10 +870,15 @@ process_remote_args (gint *argc, gchar **argv[], gboolean have_gtk, XpadSettings
 			xpad_pad_group_close_all (pad_group);
 		}
 
-		if (have_gtk && option_toggle) {
+		if (have_gtk && option_toggle_all) {
 			xpad_pad_group_toggle_hide (pad_group);
 		}
 
+		if (have_gtk && option_toggle_pads) {
+			xpad_pad_group_toggle_hide_show (pad_group);
+		}
+
+
 		if (!option_hide && !option_show) {
 			guint display_pads;
 			g_object_get (xpad_settings, "autostart-display-pads", &display_pads, NULL);
@@ -938,5 +918,5 @@ process_remote_args (gint *argc, gchar **argv[], gboolean have_gtk, XpadSettings
 	g_option_context_free (context);
 
 	return(option_new || option_quit || option_smid || option_files ||
-	       option_hide || option_show || option_toggle);
+	       option_hide || option_show || option_toggle_all || option_toggle_pads );
 }
diff --git a/src/xpad-pad-group.c b/src/xpad-pad-group.c
index 2fd9d19..d9f0989 100644
--- a/src/xpad-pad-group.c
+++ b/src/xpad-pad-group.c
@@ -223,7 +223,27 @@ xpad_pad_group_toggle_hide (XpadPadGroup *group)
 
 	xpad_pad_group_close_all (group);
 }
+ 
+/* Toogle individual note show/hide status */
+void
+xpad_pad_group_toggle_hide_show (XpadPadGroup *group)
+{
+	if (!group) {
+		return;
+	}
 
+	GSList *pad = g_slist_nth (group->priv->pads, 0);
+
+	while (pad != NULL) {
+		if (gtk_widget_get_visible (GTK_WIDGET(pad->data))) {
+			xpad_pad_close (pad->data);
+		}
+		else {
+			gtk_widget_show (GTK_WIDGET (pad->data));
+		}
+		pad = g_slist_next (pad);
+	}
+}
 
 void
 xpad_pad_group_update_sticky (XpadPadGroup *group, gboolean is_sticky)
diff --git a/src/xpad-pad-group.h b/src/xpad-pad-group.h
index 72321aa..3112411 100644
--- a/src/xpad-pad-group.h
+++ b/src/xpad-pad-group.h
@@ -60,6 +60,7 @@ void     xpad_pad_group_remove                       (XpadPadGroup *group, GtkWi
 void     xpad_pad_group_close_all                    (XpadPadGroup *group);
 void     xpad_pad_group_show_all                     (XpadPadGroup *group);
 void     xpad_pad_group_toggle_hide                  (XpadPadGroup *group);
+void     xpad_pad_group_toggle_hide_show             (XpadPadGroup *group);
 GSList*  xpad_pad_group_get_pads                     (XpadPadGroup *group);
 GSList*  xpad_pad_group_get_pads_sorted_by_title     (XpadPadGroup *group);
 guint    xpad_pad_group_num_visible_pads             (XpadPadGroup *group);
diff --git a/src/xpad-pad.c b/src/xpad-pad.c
index e90cd79..fcf93d2 100644
--- a/src/xpad-pad.c
+++ b/src/xpad-pad.c
@@ -846,18 +846,6 @@ xpad_pad_close (XpadPad *pad)
 {
 	gtk_widget_hide (GTK_WIDGET (pad));
 
-	/*
-	 * If no tray and this is the last pad, we don't want to record this
-	 * pad as closed, we want to start with just this pad next open.  So
-	 * quit before we record.
-	 */
-	if (!xpad_tray_has_indicator () &&
-		 xpad_pad_group_num_visible_pads (pad->priv->group) == 0)
-	{
-		xpad_app_quit ();
-		return;
-	}
-
 	if (pad->priv->properties)
 		gtk_widget_destroy (pad->priv->properties);