GUACAMOLE-192: Double and triple click function #406

Saiyeux · 2022-12-21T03:23:27Z

Two files added to /src/protocols/ssh and small changes to input.c. Now we can double click a character and select the whole word. Also triple to the whole line.

mike-jumper

Thanks - please take a look over this initial review and apply the feedback broadly. I have not yet read through the full internals of these proposed changes, however it looks like there is some major restructuring needed first. There are a few points with respect to style, documentation, etc. which I've made in individual locations but which apply elsewhere here. For the sake of brevity, I've only made those comments once.

mike-jumper · 2022-12-21T04:43:01Z

src/protocols/ssh/input.c

@@ -58,7 +174,7 @@ int guac_ssh_user_key_handler(guac_user* user, int keysym, int pressed) {

    /* Report key state within recording */
    if (ssh_client->recording != NULL)
-        guac_recording_report_key(ssh_client->recording,
+        guac_common_recording_report_key(ssh_client->recording,


This should remain guac_recording_report_key(). There is no guac_common_recording_report_key() function.

mike-jumper · 2022-12-21T04:44:30Z

src/protocols/ssh/input.c

-                guac_terminal_get_columns(terminal),
-                guac_terminal_get_rows(terminal));
+                terminal->term_width, terminal->term_height);


This shouldn't change to terminal->term_width, etc. which are now internal members of the terminal struct. The getter functions are the public interface.

mike-jumper · 2022-12-21T04:45:26Z

src/protocols/ssh/input.c

+long long getSystemTime() {
+    struct timeb t;
+    ftime(&t);
+    return 1000 * t.time + t.millitm;
+}


You do not need to declare your own function for this - libguac provides guac_timestamp_current().

mike-jumper · 2022-12-21T04:46:18Z

src/protocols/ssh/input.c

+int state = 0;
+long long start_1, start_2, start_3 = 0;
+long long end_1, end_2, end_3 = 0;


Global state should be avoided where possible. This would make more sense within the state representation of the internal terminal struct, and needs to be documented.

mike-jumper · 2022-12-21T04:48:40Z

src/protocols/ssh/click.h

+typedef struct guac_click {
+
+    int select_row;
+
+    int select_col;
+
+    int select_head;
+
+    int select_tail;
+
+    guac_terminal* term;
+
+    guac_client* client;
+
+    guac_socket* socket; 
+
+    guac_layer* select_layer;
+
+} guac_click;
+
+/* Display selection effect */
+void guac_click_draw_select(guac_click* click);
+
+void guac_click_draw_blank(guac_click* click);


All functions, parameters, return values, structures, and members need to be documented.

This would make more sense as part of the terminal, rather than being SSH-specific. In that case, each of these functions and structures should be renamed to be within the guac_terminal_ namespace.

mike-jumper · 2022-12-21T04:51:41Z

src/protocols/ssh/click.c

+
+    }
+
+    /* Marks */


Same here - what do you mean by "marks"?

mike-jumper · 2022-12-21T04:52:25Z

src/protocols/ssh/click.c

+    /* Reset flag */
+    flag = sentinel;


Please rephrase to capture the intent of what's happening here, rather than the literal English equivalent of the relevant statement.

mike-jumper · 2022-12-21T04:52:38Z

src/protocols/ssh/click.c

+    int sentinel = term->display->operations[row * term->display->width + col].character.value;
+    int flag = sentinel;
+
+    /* Get head */


What do you mean by "head"?

mike-jumper · 2022-12-21T04:53:11Z

src/protocols/ssh/click.c

+    printf("Head: %d\n", head);
+    printf("Tail: %d\n", tail);


Please do not include debug printf() calls.

mike-jumper · 2022-12-21T04:55:17Z

src/protocols/ssh/click.c

+    guac_protocol_send_rect(click->socket, click->select_layer, 0, 0, 1, 1);
+
+    guac_protocol_send_cfill(click->socket, GUAC_COMP_SRC, click->select_layer,
+    0x00, 0x00, 0x00, 0x00);


Why does the double/triple click behavior need its own dedicated selection redraw code, its own dedicated select layer, etc.? Can the existing terminal selection code not be used as-is?

Saiyeux · 2022-12-21T05:52:39Z

Thanks for these replies, I will read each suggestions and make reasonable changes.

Tried out another way without adding other files to realize the function.

Formatting changes.

Saiyeux · 2022-12-26T08:27:14Z

The newest commit with SHA of 9ae48ae should meet these needs mentioned above. I tried to resolve these problems by adding members to terminal structure. If this request is still available for GUACAMOLE-192, please let me know other potential hazards or format problems. Thanks for replying. @mike-jumper

Debug output deleted.

necouchman · 2023-03-19T18:57:32Z

@Saiyeux : Actual code changes aside, you have a couple of issues with your pull request:

You have a merge commit that should not be present. You'll need to rebase off the current master and get rid of that.
You have have merge conflicts that need to be resolved with the current master before these changes will commit.
All of your commit messages are exactly the same, and do not provide good descriptions of the nature of the commit. You'll need to update the commit messages, and possibly squash commits together where it makes sense.

GUACAMOLE-192: Double and triple click function

6ed9dc7

mike-jumper requested changes Dec 21, 2022

View reviewed changes

Saiyeux added 3 commits December 22, 2022 18:01

Merge branch 'apache:master' into Double_click_fix

5b591c1

GUACAMOLE-192: Double and triple click function

dc132eb

Tried out another way without adding other files to realize the function.

GUACAMOLE-192: Double and triple click function

fa3d54b

Formatting changes.

GUACAMOLE-192: Double and triple click function

9ae48ae

Debug output deleted.

corentin-soriano mentioned this pull request Apr 15, 2024

GUACAMOLE-192: Select word in terminal on double click #504

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GUACAMOLE-192: Double and triple click function #406

GUACAMOLE-192: Double and triple click function #406

Saiyeux commented Dec 21, 2022

mike-jumper left a comment

mike-jumper Dec 21, 2022

mike-jumper Dec 21, 2022

mike-jumper Dec 21, 2022

mike-jumper Dec 21, 2022

mike-jumper Dec 21, 2022

mike-jumper Dec 21, 2022

mike-jumper Dec 21, 2022

mike-jumper Dec 21, 2022

mike-jumper Dec 21, 2022

mike-jumper Dec 21, 2022

Saiyeux commented Dec 21, 2022

Saiyeux commented Dec 26, 2022 •

edited

necouchman commented Mar 19, 2023

GUACAMOLE-192: Double and triple click function #406

Are you sure you want to change the base?

GUACAMOLE-192: Double and triple click function #406

Conversation

Saiyeux commented Dec 21, 2022

mike-jumper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saiyeux commented Dec 21, 2022

Saiyeux commented Dec 26, 2022 • edited

necouchman commented Mar 19, 2023

Saiyeux commented Dec 26, 2022 •

edited