Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't see ip address in tray tooltip #666

Closed
raddyst opened this issue Jan 17, 2024 · 8 comments
Closed

Can't see ip address in tray tooltip #666

raddyst opened this issue Jan 17, 2024 · 8 comments
Labels

Comments

@raddyst
Copy link

raddyst commented Jan 17, 2024

When i connect with https://github.com/zhovner/zaborona_help/blob/master/openvpn-client-config/srv0.zaborona-help-UDP-no-encryption_maxroutes.ovpn profile,

image

i can't see ip address in tray tooltip.

image

ps. With other profiles - all ok, win7 x64

@cron2
Copy link
Contributor

cron2 commented Jan 17, 2024

@selvanair do we have a message buffer for the whole message which fills up for over-long profile names?

@cron2 cron2 added the bug label Jan 17, 2024
@selvanair
Copy link
Collaborator

When i connect with https://github.com/zhovner/zaborona_help/blob/master/openvpn-client-config/srv0.zaborona-help-UDP-no-encryption_maxroutes.ovpn profile,

The total text length allowed in Windows "notifyicon" tooltip is limited to 128 characters so truncation happens when the profile name is too long. As the balloon message has more space (256 characters) and less information, truncation is less likely there.

Double click on tray icon to open the status window and see the full IP address.

@cron2

@selvanair do we have a message buffer for the whole message which fills up for over-long profile names?

As space s limited, we do truncate some strings used to build up the tooltip, but in this case the truncation is due to Windows limit.

When there are multiple profiles connected, we try to list all connected/connecting profiles truncating both lists at ~100 characters. The total message would still get truncated by Windows to 128. When there is only one profile connected (the case above), we add "IP address" and "connected since" info as well.

In the latter case, to avoid truncating the IP, we could truncate the profile name to about 32 characters but that works for v4 addresses only.

selvanair added a commit to selvanair/openvpn-gui that referenced this issue Feb 10, 2024
Built-in tray notification icon has a tip text length limit of 128
characters which is often limited for showing the connected profile name,
connected since time and IP addresses. If the profile name is long the IP
numbers could get truncated.

Fix by using a custom tooltip window and display it when mouse hovers over
the icon. As the status bar need not be at the bottom f the screen (could be
at right, left or top as well), the location of the window is chosen based
on the muse co-ordinates that trigger the hover event.

In case of errors while setting up the tooltip window, fall back to the current
behaviour.

If the message is too long to include time and IP, truncate the profile name
part of the message.

Fixes issue OpenVPN#666

Signed-off-by: Selva Nair <[email protected]>
@selvanair
Copy link
Collaborator

PR #670 proposes a fix to this using a custom tooltip window to show the text. If the text is too long [*], the profile name part is truncated to ensure the IP address and time will be always displayed in full.

Tested on Win10 with status bar at bottom, right, top and left. Untested on multiple monitors.

[*] I set it at 500 characters but could be made longer. The built-in tooltip is restricted to 128 characters which is too small to show the IP, time and anything more than a short profile name.

selvanair added a commit to selvanair/openvpn-gui that referenced this issue Feb 11, 2024
Built-in tray notification icon has a tip text length limit of 128
characters which is often limited for showing the connected profile name,
connected since time and IP addresses. If the profile name is long the IP
numbers could get truncated.

Fix by using a custom tooltip window and display it when mouse hovers over
the icon. As the status bar need not be at the bottom of the screen (could be
at right, left or top as well), the location of the window is chosen based
on the mouse co-ordinates that trigger the hover event.

In case of errors while setting up the tooltip window, fall back to the current
behaviour.

If the message is too long to include time and IP, truncate the profile name
part of the message.

Fixes issue OpenVPN#666

Signed-off-by: Selva Nair <[email protected]>
selvanair added a commit to selvanair/openvpn-gui that referenced this issue Feb 11, 2024
Built-in tray notification icon has a tip text length limit of 128
characters which is often limited for showing the connected profile name,
connected since time and IP addresses. If the profile name is long the IP
numbers could get truncated.

Fix by using a custom tooltip window and display it when mouse hovers over
the icon. As the status bar need not be at the bottom of the screen (could be
at right, left or top as well), the location of the window is chosen based
on the mouse co-ordinates that trigger the hover event.

In case of errors while setting up the tooltip window, fall back to the current
behaviour.

If the message is too long to include time and IP, truncate the profile name
part of the message.

Fixes issue OpenVPN#666

Signed-off-by: Selva Nair <[email protected]>
@lstipakov
Copy link
Member

Hi,

On Windows 11 there are two issues;

  • When there is no connection, there is no tooltip at all. Currently we display OpenVPN GUI there.
  • When there is active connection, tooltip is displayed on top left. Currently we display it next to the tray.
    Näyttökuva 2024-02-12 110020

Näyttökuva 2024-02-12 110139

@selvanair
Copy link
Collaborator

selvanair commented Feb 12, 2024

On Windows 11 there are two issues;

I'm not totally surprised as I had to do a lot of trial and error to get this work on Windows 10. Use of a tooltip window for on demand display at a specified location is not well documented, and is a bit hackish. At the same time using the tooltip class for this window feels compelling

  • When there is no connection, there is no tooltip at all. Currently we display OpenVPN GUI there.

On Windows 10 I had this problem when the added text is empty (only title remains). Adding a as the text fixed it. Win11 is probably more picky.

  • When there is active connection, tooltip is displayed on top left. Currently we display it next to the tray.

This is strange --- I'll look for a Win11 machine to test and sort this out.

Works well on Win10 -- sigh..
Screenshot from 2024-02-12 10-43-12-1

selvanair added a commit to selvanair/openvpn-gui that referenced this issue Feb 13, 2024
Built-in tray notification icon has a tip text length limit of 128
characters which is often limited for showing the connected profile name,
connected since time and IP addresses. If the profile name is long the IP
numbers could get truncated.

Fix by using a custom tooltip window and display it when mouse hovers over
the icon. As the status bar need not be at the bottom of the screen (could be
at right, left or top as well), the location of the window is chosen based
on the mouse co-ordinates that trigger the hover event.

In case of errors while setting up the tooltip window, fall back to the current
behaviour.

If the message is too long to include time and IP, truncate the profile name
part of the message.

v2: Do not use wParam in NIN_POPUOPEN message as it does not seem to work
    on Windows 11. Instead use GetCursorPos() for mouse location.

Fixes issue OpenVPN#666

Signed-off-by: Selva Nair <[email protected]>
@lstipakov
Copy link
Member

Hi,

Looks good on Windows 11. A few nick-picks:

  • The tooltip is placed next to cursor, and not above the tray area as before. But I am not sure if it worth to fix the placement.
  • The title is in bold, unlike before. Is this intended behavior? It looks okayish when we're in connected state and tooltip contains other information, but in the idle state it looks a bit odd, especially when other apps' tooltips are not bold.

Here is a small patch which makes title part of message to remove boldness and moves tip_msh from global to local scope (why it was in global btw?):

 diff --git a/tray.c b/tray.c
index 9e86f53..eed3054 100644
--- a/tray.c
+++ b/tray.c
@@ -50,7 +50,6 @@ HBITMAP hbmpConnecting;
 NOTIFYICONDATA ni;
 HWND traytip; /* handle of tooltip window for tray icon */
 TOOLINFO ti;  /* global tool info structure for tool tip of tray icon*/
-WCHAR tip_msg[500]; /* global buffer for tray tip message */
 
 extern options_t o;
 
@@ -515,9 +514,8 @@ ShowTrayIcon()
         ti.uId = (UINT_PTR) traytip;
         ti.uFlags = TTF_ABSOLUTE|TTF_TRACK|TTF_IDISHWND;
         ti.hwnd = o.hWnd;
-        ti.lpszText = L" ";
+        ti.lpszText = _T(PACKAGE_NAME);
         SendMessage(traytip, TTM_ADDTOOL, 0, (LPARAM)&ti);
-        SendMessage(traytip, TTM_SETTITLE, TTI_NONE, (LPARAM) _T(PACKAGE_NAME));
         SendMessage(traytip, TTM_SETMAXTIPWIDTH, 0, (LPARAM) cx);
     }
 }
@@ -525,6 +523,7 @@ ShowTrayIcon()
 void
 SetTrayIcon(conn_state_t state)
 {
+    TCHAR tip_msg[500] = L"";
     TCHAR msg_connected[100];
     TCHAR msg_connecting[100];
     BOOL first_conn;
@@ -534,7 +533,7 @@ SetTrayIcon(conn_state_t state)
     _tcsncpy(msg_connected, LoadLocalizedString(IDS_TIP_CONNECTED), _countof(msg_connected));
     _tcsncpy(msg_connecting, LoadLocalizedString(IDS_TIP_CONNECTING), _countof(msg_connecting));
 
-    tip_msg[0] = L'\0';
+    _tcsncpy(tip_msg, _T(PACKAGE_NAME), _countof(_T(PACKAGE_NAME)));
     first_conn = TRUE;
     for (connection_t *c = o.chead; c; c = c->next)
     {

If you think we really need bold then I am fine with it.

@selvanair
Copy link
Collaborator

selvanair commented Feb 13, 2024

Good suggestion. In fact I had the same thought of including the title as a part of the message. Simplifies fallback to ni.szTip as well. Will push a commit.

Positioning a fixed amount above the icon looks tricky as the icon location is not returned in NIN_POPUPOPEN message -- also, even stock icons behave in all manners on Windows 10. I'll leave it like this for now, open to improvement if anyone feels like digging into it.

tip_msg has to be global (or a string literal) though. Otherwise ti.lpszText will be dangling once out of this function. This is not well-documented and my first version had this bug and occasional misbehaviour -- learned the hard way :) But kind of obvious as we assign it in one function and use it elsewhere.
Well that's what I thought at the time... Looks like the bug I had was something else and no need for a global. The pointer is referred to only when calling TTM_ADDTOOL and TTM_UPDATETIPTEXT.

Will change the patch.

This was not needed when we used ni.szTip earlier as that is a 128 character buffer in the ni structure.

@lstipakov
Copy link
Member

lstipakov commented Feb 14, 2024

. Looks like the bug I had was something else and no need for a global. The pointer is referred to only when calling TTM_ADDTOOL and TTM_UPDATETIPTEXT.

Yeah but SendMessage is synchronous, so the buffer should only be valid for the duration of the call.

Anyway, PR is now approved and ready to merge,

selvanair added a commit that referenced this issue Feb 14, 2024
Built-in tray notification icon has a tip text length limit of 128
characters which is often limited for showing the connected profile name,
connected since time and IP addresses. If the profile name is long the IP
numbers could get truncated.

Fix by using a custom tooltip window and display it when mouse hovers over
the icon. As the status bar need not be at the bottom of the screen (could be
at right, left or top as well), the location of the window is chosen based
on the mouse co-ordinates that trigger the hover event.

In case of errors while setting up the tooltip window, fall back to the current
behaviour.

If the message is too long to include time and IP, truncate the profile name
part of the message.

v2: Do not use wParam in NIN_POPUOPEN message as it does not seem to work
    on Windows 11. Instead use GetCursorPos() for mouse location.

Fixes issue #666

Signed-off-by: Selva Nair <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants