Add tooltip for the compass widget

[sebastien-rosset]

Changes in this PR

1. Display a tooltip when the user mouses over the compass/gnss widget (after the standard ~1.5s delay). It wasn't very obvious what the blue/red/pink colors mean, especially when the user is trying OpenCPN without being connected to a GPS receiver.
2. Fix #4211.

North up (Ubuntu desktop style):

Course up:

Heading up:

Dual canvas layout:

About the tooltip look and feel

The tooltips are rendered using the wxWidget library, which generally aims to provide the native UI look and feel (Windows, MacOS, Linux desktop UI). This means tooltip rendering is not the same across all platforms. This behavior is consistent with tooltips in the grib plugin. For example, see below "Hide data at cursor" tooltip styles on Mac versus Linux.

On MacOS:

On Ubuntu with the default desktop:

[bc38]

It looks nice, now we need to make the rest of the tips in the same style. You do not take into account screen scaling at high resolutions.

[sebastien-rosset]

It looks nice, now we need to make the rest of the tips in the same style. You do not take into account screen scaling at high resolutions.

Are you referring to the dark background versus white background for the tooltips? Is there a style guideline for tooltips?

[bc38]

It looks nice, now we need to make the rest of the tips in the same style. You do not take into account screen scaling at high resolutions.

Are you referring to the dark background versus white background for the tooltips? Is there a style guideline for tooltips?

Yes, that's exactly what we're talking about! The style as such is not described in the rules, but tooltips with a dark background and rounded edges are close in the general style of the interface

The only thing is the background should be smaller, otherwise it takes up a lot of space

[sebastien-rosset]

Tooltips in the tool bar use white background. Tooltips in the grib tool use the same dark background:

The only thing is the background should be smaller, otherwise it takes up a lot of space

Is this really a problem? Tooltips only show up if you wait about one second, and they disappears if you move the mouse. Presumably the style would have to be done in a central place such that they are consistent across all widgets, i.e. the tooltips for the grib widget have the same style.

I see the toolbar uses a custom tooltip. https://github.com/OpenCPN/OpenCPN/blob/2229ce1e94d3fc9d420b7ece0cdba017341193cc/gui/src/toolbar.cpp#L1399-L1424 based on the ToolTipWin class: https://github.com/OpenCPN/OpenCPN/blob/2229ce1e94d3fc9d420b7ece0cdba017341193cc/gui/src/toolbar.cpp#L810

Whereas the grib widget uses regular wx tooltips from the wxwidget library.

[sebastien-rosset]

You do not take into account screen scaling at high resolutions.

Can you elaborate on what you would like to see? I used the same wxToolTip widget which is used in the grib plugin, e.g., https://github.com/OpenCPN/OpenCPN/blob/2229ce1e94d3fc9d420b7ece0cdba017341193cc/plugins/grib_pi/src/CursorData.cpp#L318.

The out-of-the-box wxToolTip customization is somewhat limited: https://docs.wxwidgets.org/latest/classwx_tool_tip.html

[rgleason]

Sebastein, I don't know if this will help, but it did help with plugins

https://github.com/OpenCPN/plugins/issues/942

[sebastien-rosset]

In the wxWidgets library, I think the principle for tooltips is to ensure that the tooltip look and feel complies with platform recommendations, rather than providing a consistent look across all platforms.

1. On Windows, tooltips will look and behave like standard Windows tooltips.
2. On macOS, tooltips will adhere to Apple's Human Interface Guidelines.
3. On various Linux desktops, tooltips will match the appearance defined by the current desktop environment or theme, e.g. Ubuntu Spatial UI Design.

https://docs.wxwidgets.org/3.2.6/classwx_rich_tool_tip.html

In OpenCPN, the tooltips are currently inconsistent. For example the tooltips in the grib plugin are not the same as the tooltips in the toolbar. The first step is to define what the behavior should be: 1) provide native platform look and feel. or 2) provide custom OpenCPN look and feel (e.g. with custom font size and color, background color, padding, day vs night, etc).

The grib plugin uses approach 1), whereas the toolbar uses approach 2). For example, it looks like the toolbar seems to follow the Apple's Human Interface Guidelines when OpenCPN runs on Ubuntu.

If we go with option 2), then I think there should be a custom tooltip class that should be used everywhere (core capabilities and plugins).

[sebastien-rosset]

Sebastein, I don't know if this will help, but it did help with plugins

OpenCPN/plugins#942

These scenarios do not cover tooltips. The size of the tooltip widgets is handled by the wxwidget library.

You do not take into account screen scaling at high resolutions.

What exactly do you expect to happen with tooltips at high resolutions? I've looked at the code elsewhere and tried at different resolutions. This tooltip behaves similar to other tooltips, in particular the same as in the grib tool. For example, here is how it looks on MacOS at 4096x2560:

[rgleason]

Sebastien, you bring up good points about improving the consistency with which Tool Tips are handled. I think it would be helpful if someone more knowledgeable about program development gave you a review, or some discussion about preferences.

[Hakansv]

I just happened to see that you've coded the tooltips so that they don't translate? Like this: tooltipText += _T("GNSS data stale"); I think the code would be like: tooltipText += _("GNSS data stale");

BTW: The use of "_T" to address "No translate" may be old fashion. This works as well: += "No translation"

[Hakansv]

I merged your PR for a test on my Win10 - VS2022. No Tooltips was shown on mouse over. Neither in single nor double canvas mode. No further debug was performed.

[Hakansv]

BTW2: Lots of comments in your code. More than "usual". Back in time, the message was that the code should be written so that you understand what is expected to happen instead of in-depth comments. Not least, variables should be named so that you understand the meaning. These ground rules may have changed slightly nowadays. The question is to which policy? The developer manual refers to the Google style. Without being mean, maybe these don'ts can be useful?

@bdbcat, Do you have time to elaborate on how we should do today?

[sebastien-rosset]

BTW2: Lots of comments in your code. More than "usual". Back in time, the message was that the code should be written so that you understand what is expected to happen instead of in-depth comments. Not least, variables should be named so that you understand the meaning. These ground rules may have changed slightly nowadays. The question is to which policy? The developer manual refers to the Google style. Without being mean, maybe these don'ts can be useful?

@bdbcat, Do you have time to elaborate on how we should do today?

I've previously added comments, such as in this PR: https://github.com/OpenCPN/OpenCPN/pull/4133.

The google style states:

Comments are absolutely vital to keeping our code readable. The following rules describe what you should comment and where. But remember: while comments are very important, the best code is self-documenting. Giving sensible names to types and variables is much better than using obscure names that you must then explain through comments.

So, yes, the best code is self documenting, but the reality is it takes quite a bit of effort to understand the precise semantic of variables/methods/classes/structs. In this specific use case, there are two related variable names bGPSValid and g_bSatValid. The names are similar but carry slightly different semantic. My take is that if I've spent a significant amount of time to understand the nuances of one variable, then hopefully it will be valuable to document and help the next maintainer.

Below I don't think I'm stating the obvious. Stating the obvious would have been a comment such as "Is satellite valid?". One piece of information is that it's the status as obtained from the watchdog timer. The second point is the definition of "valid", which in this particular case means no GNSS data has been received (as opposed to for example the GNSS data being received and somehow invalid).

// Indicate the status of the satellite watchdog timer.
// - true if GNSS satellite data has been received recently.
// - false if no GNSS satellite data has been received recently.
bool g_bSatValid;

[sebastien-rosset]

I merged your PR for a test on my Win10 - VS2022. No Tooltips was shown on mouse over. Neither in single nor double canvas mode. No further debug was performed.

I've been able to reproduce. Somehow it depends on the screen resolution. At lower resolutions I see the compass rectangle X coordinate is not what it should be, whereas Y, width and height are correct. I will investigate.

As a related point, I've noticed a green triangle is displayed when hovering over the compass widget. These arrows are not displayed when hovering over the menu bar and the lower chart panel option. IMO, the behavior should be the same as the menu bar, i.e., the green triangle should not be displayed when hovering over the compass widget.

[sebastien-rosset]

I just happened to see that you've coded the tooltips so that they don't translate? Like this: tooltipText += _T("GNSS data stale"); I think the code would be like: tooltipText += _("GNSS data stale");

BTW: The use of "_T" to address "No translate" may be old fashion. This works as well: += "No translation"

I've made the change. I'm unclear what to do to generate the .pot and .po files. make pot-update fails make po-update generates a lot of changes.

I raised #4210 to fix the make pot-update target. Once #4210 is merged, I'll be able to run make pot-update in this branch.

[sebastien-rosset]

I merged your PR for a test on my Win10 - VS2022. No Tooltips was shown on mouse over. Neither in single nor double canvas mode. No further debug was performed.

I think this is related to #4211. I see the x coordinate of the compass is wrong depending on the resolution. When you hover over the compass widget, do you see a green triangle or the arrow pointer?

[Hakansv]

I see the string changes so now these texts will be present in the pot-fil. Good. The "pot-update" project runs nicely on my VS2022 when needed. It's used to produce the ...\OpenCPN\po\opencpn.pot when it's time to update Crowdin with new strings for translation before a new O release. No need for the updated .pot file in beforehand. The po-files are produced by Crowdin, when the translators are ready, and copied to O source. The po-update I've never run.

[Hakansv]

No tooltips shown: I'm on Win10 and the Windows screen scaling is 175%. No green triangle or arrow pointer seen when "mouse over"

[Hakansv]

// Indicate the status of the satellite watchdog timer. // - true if GNSS satellite data has been received recently. // - false if no GNSS satellite data has been received recently. bool g_bSatValid;

In this case I think you have to be careful to not confuse others. Your text above is not completely true. g_bSatValid is about how many satellites are seen and also what N0183, Signalk and N2000 has reported about a valid parse of the GNSS signals. It may be to complicated to describe with one or two rows of comments. It's about a valid GNNS reception, That's clear from the names. May be enough?

[sebastien-rosset]

I see the string changes so now these texts will be present in the pot-fil. Good. The "pot-update" project runs nicely on my VS2022 when needed. It's used to produce the ...\OpenCPN\po\opencpn.pot when it's time to update Crowdin with new strings for translation before a new O release. No need for the updated .pot file in beforehand. The po-files are produced by Crowdin, when the translators are ready, and copied to O source. The po-update I've never run.

make pot-update fails on mac, I've raised #4210

[sebastien-rosset]

// Indicate the status of the satellite watchdog timer. // - true if GNSS satellite data has been received recently. // - false if no GNSS satellite data has been received recently. bool g_bSatValid;

In this case I think you have to be careful to not confuse others. Your text above is not completely true. g_bSatValid is about how many satellites are seen and also what N0183, Signalk and N2000 has reported about a valid parse of the GNSS signals. It may be to complicated to describe with one or two rows of comments. It's about a valid GNNS reception, That's clear from the names. May be enough?

The meaning of g_bSatValid was definitely not clear to me while working on this PR. I can work to improve the code comments. The UI representation was also not clear. My crew and I were confused by the meaning of the satellite status in the compass widget.

Here is a possible improvement:

/**
 * Indicates valid GNSS reception status based on satellite visibility
 * and successful parsing of NMEA0183, SignalK, or NMEA2000 data.
 * Reset to false if no valid signal is received within watchdog timeout period.
 */
bool g_bSatValid;

SatValid could reasonably mean:

1. Is there a valid satellite fix/position
2. Are there enough satellites visible for navigation
3. Is satellite data being received successfully
4. Is the satellite subsystem functioning properly

So without comments, I might be unsure whether this indicates:

1. A basic "do we have satellites?" status
2. A more specific "do we have enough satellites for a good fix?"
3. A general "is our satellite navigation system working properly?"
4. Just satellite visibility vs actual valid position data

The name is decent for a quick understanding that it's about satellite validity, but without comments I wouldn't know about:

1. The multiple protocol sources (NMEA0183, SignalK, NMEA2000)
2. The watchdog timeout behavior
3. What exactly constitutes "valid" (visibility? good fix? parsed data?)

[Hakansv]

About GNSS Compass mode. A good start may be to read the manual? I agree some comments may be helpful and also appreciate your efforts to this but the comments have to be rather short and not the "whole story". That's if you ask me... In VS2022 i use to "Find all references" to find out how and when it's set. That function would be present in other tools as well?

[sebastien-rosset]

I agree some comments may be helpful and also appreciate your efforts to this but the comments have to be rather short and not the "whole story". That's if you ask me...

In other PRs, reviewers seemed to encourage my effort to provide better doxygen documentation. I find it's generally easier for community members to contribute when code is well documented. New developers need context beyond just the implementation, this reduces reliance on tribal knowledge. You seem to have a different point of view, where 2 sentences of code comments are too much (almost all my comments are 2 sentences).

In the Google style guide:

All global variables should have a comment describing what they are, what they are used for, and (if unclear) why they need to be global.

Before PR: global variables had no code comments. In this PR: I've added code comments, per OpenCPN guidance to use the Google style guide.

Almost every function declaration should have comments immediately preceding it that describe what the function does and how to use it. These comments may be omitted only if the function is simple and obvious (e.g., simple accessors for obvious properties of the class). Private methods and functions declared in .cc files are not exempt. Function comments should be written with an implied subject of This function and should start with the verb phrase; for example, "Opens the file", rather than "Open the file". In general, these comments do not describe how the function performs its task. Instead, that should be left to comments in the function definition.

Before PR: some function declarations did not have code comments. In this PR: I've added code comments to document the functions, such that they are included in doxygen.

In your implementation you should have comments in tricky, non-obvious, interesting, or important parts of your code.

Before PR: There was a bug caused by a confusion between physical pixels versus logical or DPI pixels. In this PR: I've added comments explaining the compass widget m_rect variable is in physical pixels, which was compared to the mouse position, which is in logical pixels. I.e. in the code below as a contributor it was not obvious that m_compass->GetRect() returns physical pixels, whereas event.GetPosition() returns logical pixels.

if (m_compass->GetRect().Contains(event.GetPosition()) { ... }

I think if the criteria is not the same as Google style guide, then an exception should be explicitly written in https://opencpn-manuals.github.io/main/opencpn-dev/coding-guidelines.html. This would help to bring consistency and speed up the PR review process. I can remove the comments, but I'd rather have clear guidelines for contributors and go with the guidelines, rather than the opinion of one reviewer.

[leamas]

the meaning of g_bSatValid was definitely not clear to me.

Indeed. The actual meaning of this without a comment is IMHO clear as mud.

I think if the criteria is not the same as Google style guide, , then an exception should be explicitly written in [...]

Yes. However, it is not -- the style guide is correct and should be applied. In particular, commenting globals is helpful.

[leamas]

/**
 * Indicates valid GNSS reception status based on satellite visibility
 * and successful parsing of NMEA0183, SignalK, or NMEA2000 data.
 * Reset to false if no valid signal is received within watchdog timeout period.
 */

This comment is fine, and doxygen will sort it out. However, the default rule is to comment headers rather than implementation i. e., this comment is better placed in comm_vars.h.

[sebastien-rosset]

This comment is fine, and doxygen will sort it out. However, the default rule is to comment headers rather than implementation i. e., this comment is better placed in comm_vars.h.

Good point, I've made the change.

[sebastien-rosset]

https://github.com/user-attachments/assets/a38e09c4-69b8-49b7-9d82-93ccba15f3fc

[Hakansv]

I tested now on Win10. Sorry no tooltip visible here.

[sebastien-rosset]

Just curious, did you wait 1.5 seconds after hovering over the compass widget, as shown in the attached video? It's working for me on Mac and Linux, but I'll try to find a Windows machine to test there. Also, your setup is configured with OpenGL, right? AFAIK, the compass is shown only when OpenGL is enabled.

[Hakansv]

OpenGl is on, yes. I can wait for minutes, no tooltips shown. Now I'm not a skilled programmer but when debug I can see the "SetToolTip(tooltipText);" is nicely set to e.g. "North up.." Any hints where I shall look to catch when the tooltips box should have been shown?