Add the `-via` arguments to config file and GUI for the viewer by dmcmahill · Pull Request #1517 · TigerVNC/tigervnc

dmcmahill · 2022-08-19T14:12:07Z

vncviewer already includes a -via <ssh_host> command line option to
specify that an SSH tunnel to <ssh_host> should be established and
VNC traffic routed through the tunnel. However, that option was not
available either in the *.tigervnc setup files or via the GUI.

This adds UseSSH (boolean) and via (string) parameters to the config
file and exposes them to the VNC viewer GUI under the options dialog.

This shows where I added the option to the GUI:

This shows an example of how the option shows up in a .tigervnc file:

UseSSH=1
via=my.sshhost.com

CendioOssman

Nice! This would make this feature more accessible to others. I have some small concerns, though. See the comments.

dmcmahill · 2022-10-18T12:46:18Z

I've reworked the code from this PR into a branch based on the work in #1529. Once that PR merges I'll rebase the branch and resubmit this PR using the new code. In the mean time, my new branch is at https://github.com/dmcmahill/tigervnc/commits/newoptions-via-to-gui

CendioOssman · 2022-10-18T13:50:10Z

Sounds good. But please don't open a new PR. Simply rebase and force push an update to the PR here. That way we keep all history.

dmcmahill · 2022-10-18T16:17:40Z

Sounds good. But please don't open a new PR. Simply rebase and force push an update to the PR here. That way we keep all history.

Understood. I went ahead and force pushed the new code that is still rooted in your branch so I'll need to do one more rebase and force push once the other PR merges. But at least this PR now shows what the change will be.

Here is the options dialog with the new entry.

CendioOssman · 2022-10-19T11:00:26Z

One goal of the new layout is to avoid having to increase the size of the options dialog, and instead have scroll bars. My idea was something like this:

diff --git a/vncviewer/OptionsDialog.cxx b/vncviewer/OptionsDialog.cxx
index 5550ade7..dd295abe 100644
--- a/vncviewer/OptionsDialog.cxx
+++ b/vncviewer/OptionsDialog.cxx
@@ -53,6 +53,7 @@
 #include <FL/Fl_Round_Button.H>
 #include <FL/Fl_Int_Input.H>
 #include <FL/Fl_Choice.H>
+#include <FL/Fl_Scroll.H>
 
 using namespace std;
 using namespace rdr;
@@ -63,7 +64,7 @@ std::map<OptionsCallback*, void*> OptionsDialog::callbacks;
 static std::set<OptionsDialog *> instances;
 
 OptionsDialog::OptionsDialog()
-  : Fl_Window(580, 480, _("TigerVNC Options"))
+  : Fl_Window(580, 420, _("TigerVNC Options"))
 {
   int x, y;
   Fl_Button *button;
@@ -668,7 +669,9 @@ void OptionsDialog::createCompressionPage(int tx, int ty, int tw, int th)
 void OptionsDialog::createSecurityPage(int tx, int ty, int tw, int th)
 {
 #if defined(HAVE_GNUTLS) || defined(HAVE_NETTLE) || !defined(WIN32)
-  Fl_Group *group = new Fl_Group(tx, ty, tw, th, _("Security"));
+  Fl_Scroll *scroll = new Fl_Scroll(tx, ty, tw, th, _("Security"));
+  tw -= Fl::scrollbar_size();
+  Fl_Group *group = new Fl_Scroll(tx, ty, tw, 0);
 
   int orig_tx;
   int width;
@@ -828,10 +831,15 @@ void OptionsDialog::createSecurityPage(int tx, int ty, int tw, int th)
   tx = orig_tx;
   ty += INNER_MARGIN;
 
-
 #endif // !WIN32
 
+  /* Needed for resize to work sanely */
+  group->resizable(NULL);
+  ty += OUTER_MARGIN - INNER_MARGIN;
+  group->size(group->w(), ty - group->y());
+
   group->end();
+  scroll->end();
 
 #endif // defined(HAVE_GNUTLS) || defined(HAVE_NETTLE) || !defined(WIN32)
 }

dmcmahill · 2022-11-18T15:27:33Z

Updated with scrollbars as suggested by @CendioOssman . Thanks!

CendioOssman · 2022-11-21T13:15:14Z

Sorry about the delay. You are not forgotten, we're just a bit too busy at the moment. Please bear with us a bit more, and we will get this merged eventually.

CendioOssman · 2022-12-22T12:46:41Z

The options dialog changes are now merged, so please have a look at rebasing this PR so we can focus on just your new additions.

vncviewer already includes a `-via <ssh_host>` command line option to specify that an SSH tunnel to `<ssh_host>` should be established and VNC traffic routed through the tunnel. However, that option was not available either in the `*.tigervnc` setup files or via the GUI. This adds a `via` (string) parameter to the config file and exposes it to the VNC viewer GUI under the options dialog. In addition, when establishing a tunnel and even before deciding to establish a tunnel, strip leading and trailing whitespace because `ssh " my.host.com "` will indeed fail and if someone manages to get a blank value (only whitespace) into the GUI or the config file it can produce confusing results.

Apply the code suggested by Pierre Ossman to use a scrollbar in the security section of the options dialog instead of growing the size of the dialog.

dmcmahill · 2022-12-22T14:22:54Z

The options dialog changes are now merged, so please have a look at rebasing this PR so we can focus on just your new additions.

rebased and force pushed

CendioOssman

Great. I have some minor things that need adjusting, then we can go ahead and merge this.

CendioOssman · 2022-12-23T08:35:50Z

+  /* Connection */
+  ty += GROUP_LABEL_OFFSET;
+  //  height = GROUP_MARGIN * 2 + (INPUT_LABEL_OFFSET + INPUT_HEIGHT) * 1;
+  connectionGroup = new Fl_Group(tx, ty, width, 0, _("Connection"));


"Connection" is a bit general, so I think we need something more specific here. "Gateway" is what's used in the documentation, so something including that word would probably be best for consistency.

CendioOssman · 2022-12-23T08:36:24Z


  static void handleX509(Fl_Widget *widget, void *data);
  static void handleRSAAES(Fl_Widget *widget, void *data);
+  static void handleViaHost(Fl_Widget *widget, void *data);


This doesn't seem to be used?

Thanks for catching this. It was there in the original version before your new options work and when I reworked the code I accidentally left this in. I'll remove it.

CendioOssman · 2022-12-23T08:38:41Z


 #ifndef WIN32
-StringParameter via("via", "Gateway to tunnel via", "");
+StringParameter via("via", "SSH host for tunnel", "");


This is an unrelated change, so it should be a separate commit.

The term "Gateway" is also used elsewhere, so everything would need to be adjusted so things are consistent.

Lastly, I'm uncertain if this is clearer. We're gaining the detail of it being SSH, but losing the gateway¹ aspect.

¹ Which is a less common description than "proxy"

I'll remove this particular change.

CendioOssman · 2022-12-23T08:39:25Z

+  while (*tmps2 != '\0' && !isspace(*tmps2)) tmps2++;
+  if (isspace (*tmps2)) *tmps2 = '\0';
+  vlog.info(_("Establish SSH tunnel to gatewayHost \"%s\"\n"), tmps);
+


These are nice improvements, but unrelated to adding the GUI. So please have this as a separate commit.

Just to make sure I understand before I do it, would you like to see me take the changes (plus your corrections) and break into the GUI part and the non GUI part, re-do the commits and force push to have a more clean history before the merge? I'm happy to do that but wanted to make sure I didn't misunderstand your request.

Yes, that's precisely right. :)

CendioOssman · 2022-12-23T08:39:47Z

+      if (useSSH && mktunnel() != 0)
+        usage(argv[0]);
+      strFree(tmps);
+    }


CendioOssman · 2022-12-23T08:40:30Z


 OptionsDialog::OptionsDialog()
-  : Fl_Window(580, 480, _("TigerVNC Options"))
+  : Fl_Window(580, 420, _("TigerVNC Options"))


Please include this commit into the main one so we can avoid this back and forth.

vejnar · 2023-03-25T15:11:12Z

This would be a fantastic feature! Thanks for working on it. PR seems almost done. Any news @dmcmahill to push for the finish line?

Question: Would this option be available on Windows?

CendioOssman · 2024-08-06T12:17:21Z

Unfortunately, it seems this PR is abandoned. :/

CendioOssman requested changes Aug 23, 2022

View reviewed changes

Comment thread vncviewer/OptionsDialog.cxx Outdated

Comment thread vncviewer/parameters.cxx Outdated

Comment thread vncviewer/OptionsDialog.cxx Outdated

dmcmahill force-pushed the add-via-to-viewer-gui branch from c34524b to 10ac587 Compare September 5, 2022 20:29

CendioOssman mentioned this pull request Sep 7, 2022

Change to modern layout in options dialog #1529

Merged

dmcmahill force-pushed the add-via-to-viewer-gui branch from 752eeb3 to e410f25 Compare October 18, 2022 16:12

dmcmahill added 2 commits December 22, 2022 09:00

Use scrollbars in the security section of the options dialog

53473dd

Apply the code suggested by Pierre Ossman to use a scrollbar in the security section of the options dialog instead of growing the size of the dialog.

dmcmahill force-pushed the add-via-to-viewer-gui branch from dfbed39 to 53473dd Compare December 22, 2022 14:21

CendioOssman requested changes Dec 23, 2022

View reviewed changes

CendioOssman closed this Aug 6, 2024

Uh oh!

Conversation

dmcmahill commented Aug 19, 2022

Uh oh!

CendioOssman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmcmahill commented Oct 18, 2022

Uh oh!

CendioOssman commented Oct 18, 2022

Uh oh!

dmcmahill commented Oct 18, 2022

Uh oh!

CendioOssman commented Oct 19, 2022

Uh oh!

dmcmahill commented Nov 18, 2022

Uh oh!

CendioOssman commented Nov 21, 2022

Uh oh!

CendioOssman commented Dec 22, 2022

Uh oh!

dmcmahill commented Dec 22, 2022

Uh oh!

CendioOssman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vejnar commented Mar 25, 2023

Uh oh!

CendioOssman commented Aug 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants