Navigator Widget (WIP) #1064
Navigator Widget (WIP) #1064
Conversation
src/Renderer/NavigatorRenderer.cpp
Outdated
| auto waypoint_direction = static_cast<int>(bearing_diff.AsDelta().Degrees()); | ||
| auto waypoint_direction_s = std::to_string(waypoint_direction); | ||
|
|
||
| const auto caption1 = waypoint_distance_s + " km | " + waypoint_altitude_diff_s + " m | " + waypoint_GR_s + ":1"; |
There was a problem hiding this comment.
Please don't concatenate strings using std::string, it's uncontrollable bloat.
There was a problem hiding this comment.
ok, amended as follows:
TCHAR informations_next_waypoint_s[100]{};
_stprintf(informations_next_waypoint_s, _T("%s | %s | %s | %s | %s"),
waypoint_distance_s, waypoint_altitude_diff_s, waypoint_GR_s, current_speed_s, waypoint_direction_s);
is it OK?
There was a problem hiding this comment.
Can you guarantee that the buffer will not overflow?
There was a problem hiding this comment.
In this case, I made a mistake, I cannot guarantee that the buffer will not overflow.
my assumption: each char array have a size of 20 --> it means 19 characters x 5 + 4 x " | " (three characters) + +1 char '\0'
result: 108 --> overflow (!)
so I will change this with appropriate length --> in this case : informations_next_waypoint_s[110] ;
There was a problem hiding this comment.
You could just use a variant of sprintf which truncates instead of overflowing the buffer. That way, you don't need an unrealistic worst-case stack allocation.
There was a problem hiding this comment.
I found the StaticString<..> very useful and easier to read ; That's why I use it now for all strings in the method NavigatorRender::drawText() ; in the background, it is using the snprintf() function which was the one I would like to use as you suggested.
as instance:
StaticString<8> time_start_s;
const auto time_start = calculated.ordered_task_stats.start.time;
if (tp == TaskType::ORDERED && has_started && basic.time_available)
time_start_s.Format("%s", FormatLocalTimeHHMM(time_start, utc_offset1).c_str());
else
time_start_s.Format("%s", "--:--");instead of:
TCHAR time_start_s[10];
const auto time_start = calculated.ordered_task_stats.start.time;
if (tp == TaskType::ORDERED && has_started && basic.time_available)
snprintf(time_start_s, ARRAY_SIZE(time_start_s), _T("%s"),
FormatLocalTimeHHMM(time_start, utc_offset1).c_str());
else
snprintf(time_start_s, ARRAY_SIZE(time_start_s), _T("%s"), "--:--");Is it OK for you ?
src/Renderer/NavigatorRenderer.cpp
Outdated
|
|
||
| // e_WP_Name | ||
| TCHAR waypoint_name_s[40]; | ||
| strncpy(waypoint_name_s, wp_current.name.c_str(), ARRAY_SIZE(waypoint_name_s)-1); |
There was a problem hiding this comment.
Codacy is right - strncpy() is a hazard and it looks like you don't know what it does. Don't use it unless you know what you're doing, aware of all the pitfalls.
There was a problem hiding this comment.
StaticString<..> + Format() method used instead of strncpy()
see #1064 (comment)
|
There hasn't been any progress in nearly half a year, and the PR branch is a big mess that is impossible to review. Let's reopen this when there is hope for some real progress. |
|
OK, thanks for your feedback. |
|
No, don't open a new PR. This will only waste our time because we don't see previous review comments. If you want to continue this feature, do it in this PR. |
|
Sorry, I don't want to waste your time. My aim is exactly the opposite : give you and the user community hours. I appreciate using XCSoar for free (!) for years. In April, I published new features and solved some issues in this PR. If I could summarize my aim participating on XCSoar project, I would use this timeline and aims: For the points (0) --> (4), I didn't want to disturb any developers and I knew that I will perhaps introduce messy/ not efficient/ or even worse stupid code. But I think, it's a good start to learn something (aka RTFM). So I took part of my free time to read developer book and XCSoar's code. |
|
I didn't even look at any of your code recently - I just looked at the list of commits. There are 12 commits, 7 of which have a commit message says "update NavigatorWidget", and 2 of them say "add NavigatorWidget". |
|
@threaderic Have a look at “git squash” - you want the 12 commits squashed into one (or however many you’ve designed to be merged in the end). Then force-push to your branch to update the PR, and repeat as and when any changes are requested and addressed. |
|
Thanks for your answers!
I will squash all these commits to one and modify information into the summary of the PR (top information) to be aligned with all the functionalities in the upcoming days. |
|
As written in the previous message, I modified the PR summary at the top to be aligned with all functionalities. I also rebased (pick and squash) and push --force. |
|
@threaderic can you re-open this very same PR? I believe it doesn't sync with your branch since it's closed. # Fetch upstream master branch of the "original" repo
git fetch upstream master
# Properly rebase your current branch on the latest master (you may have to fix conflicts)
git rebase upstream/master
# (Optional) Rewrite the last commit message
git commit --amend -m "A better commit message"
# Set the remote exactly as your local
git push --forceOnce done you should be good, provide a good commit message. |
|
@elgandoz thanks for your detailled answer ! @MaxKellermann could you please reopen the PR, I don't have the right to do this. As wrote by @elgandoz, I will rebase my current branch on the latest master. |
|
Can't reopen unless your branch has the same commit id as when it was closed, i.e. 79eef1c - this is a braindead GitHub limitation. |
|
@MaxKellermann OK, I find a way to put the same commit id as when it was closed on the NavigationWidget branch. 79eef1c |
threaderic
force-pushed
the
NavigationWidget
branch
2 times, most recently
from
56a2c5e
to
8d89ba2
Compare
threaderic
force-pushed
the
NavigationWidget
branch
2 times, most recently
from
0479b0e
to
5620607
Compare
threaderic
force-pushed
the
NavigationWidget
branch
from
5620607
to
6f2aeb7
Compare
|
last month, I successfully update the PR with the last changes of XCSoar main repo. @MaxKellerman, @DanD222 and @elgandoz: did you have time to check my code? Is it possible to have some helps? some thoughts to improve my code? I would like to close earlier questions in the PR, in order to clean the code (erase lots of pointless commented code). could you answer these opened questions in the PR:
Here some of minor issues and code I would appreciate to improve:
|
threaderic
force-pushed
the
NavigationWidget
branch
from
6f2aeb7
to
082bd77
Compare
.gitignore
Outdated
| # VSCodium | ||
| .cache/ | ||
| .vscode/ | ||
| compile_commands.json |
There was a problem hiding this comment.
This doesn't belong here
There was a problem hiding this comment.
If OK for you, I will delete these lines at the end of the review. It helps me (a lot) not always doing cooking thinks (i.e copy paste delete).
Perhaps do you have any idea doing this in an other way?
There was a problem hiding this comment.
This shouldn't be checked in.
And if it should be a separate commit explaining why you want this.
There was a problem hiding this comment.
I actually use VSCodium / VSCode.
The folders + file compile_commands.json are facilities/tools.
@lordfolken: if I understood well, you want me to create a new PR + commit for this modification?
There was a problem hiding this comment.
Those .gitignore entries are specific to your toolchain, so there’s no reason to include them in the XCSoar codebase
(“ This shouldn't be checked in.” - this means they shouldn’t have been added to git on your machine in the first place)
“ And if it should… be a separate commit” - this means that in cases you think changes similar to this are needed, this change should be in its own separate commit, and not within an “Add Navigator Widget” commit.
To answer your question, I’m pretty sure lordfolken is
- asking you to not include your changes to .gitignore in pull requests to XCSoar
- giving you hints on how to make changes that are functionally-specific (they should be in their own commit)
There was a problem hiding this comment.
I actually use VSCodium / VSCode. The folders + file compile_commands.json are facilities/tools.
Unfortunately that software really doesn't help in using git. It just marks all the files modified and just adds it to one commit.
Select the files individually and commit what belongs together.
There was a problem hiding this comment.
I finally find a solution. I keep the .gitignore locally modified and don't push it anymore.
I will create a PR to add this .gitignore modification separately in one specific commit.
| @@ -122,8 +124,20 @@ GaugesConfigPanel::Prepare(ContainerWindow &parent, | |||
| AddBoolean(_("Vario bar"), | |||
| _("If set to ON the vario bar will be shown"), | |||
| map_settings.vario_bar_enabled); | |||
|
|
|||
There was a problem hiding this comment.
Why do you suggest removing this line and why is this change hidden in this unrelated commit?
There was a problem hiding this comment.
I suggested removing this line to be in accordance with all lines declarations above, i.e. one "block of lines" for each parameter set as expert row:
add...(_("Something"),
_("Description Something"),
...);
SetExpertRow(Something);
I keep this logic for the 2 additional parameters added for the navigator widget settings (only appear in expert mode).
| AddInteger( | ||
| _("Navigator Height"), | ||
| _("Select the height of the navigator topwidget (percentage of the main window).\n\n" | ||
| "This widget is set in the System Menu --> Look --> Pages --> Top Area\n\n" |
There was a problem hiding this comment.
No ASCII art in messages, please. And no double "\n\n", attempting to layout the text.
There was a problem hiding this comment.
ASCII art replaced ('/' instead of "-->") and all double '\n' removed:
"Select the height of the navigator topwidget (percentage of the main window).\n"
"This widget is set under the settings menu: System / Look / Pages / Top Area\n"
OK for you?
| _("Navigator Height"), | ||
| _("Select the height of the navigator topwidget (percentage of the main window).\n\n" | ||
| "This widget is set in the System Menu --> Look --> Pages --> Top Area\n\n" | ||
| "Warning: a big size could lead to a bad presentation of the datas.\n" |
There was a problem hiding this comment.
"datas" is not clear enough...
I changed it to:
"Warning: an unsuitable height of the navigator widget could lead to a bad presentation of its included flight informations (e.g. name of waypoint, times, ...).\n"
| // TCHAR time_elapsed_s[10]; | ||
| // const auto time_elapsed_s_tmp = | ||
| // FormatLocalTimeHHMM(time_elapsed, utc_offset1).c_str(); | ||
| // if (has_started) | ||
| // snprintf(time_elapsed_s, ARRAY_SIZE(time_elapsed_s), _T("%s"), | ||
| // time_elapsed_s_tmp); | ||
| // else | ||
| // snprintf(time_elapsed_s, ARRAY_SIZE(time_elapsed_s), _T("%s"), "--:--"); |
There was a problem hiding this comment.
That comments comes from this question
if OK, I could delete all related comments (lots of commented blocks with use of snprintf)
threaderic
force-pushed
the
NavigationWidget
branch
from
082bd77
to
5db755e
Compare
|
Can you please resolve the codacy issues, and also resolve any conversations (hitting the button) you think you resolved. |
|
In general this change is too huge for one commit. Please build building blocks that compile and build step by step. Otherwise its very hard to review or merge or comment. Lets divide and conquer. |
|
As a first thing we should get the window managment right. |
|
@lordfolken: thanks for your comments. I checked and closed all resolved conversations. in your comment, you wrote: In general this change is too huge for one commit. The divide & conquer method is another method but I'm not sure understanding it very well. For information, the whole code in this PR is building and working under my local ubuntu 22.04 since the beginning. make -j12 TARGET=UNIX USE_CCACHE=y SANITIZE=n DEBUG=n WERROR=nI checked why it was not passing the build under the github workflow processes by using the variants build-ubuntu and build-sanitizer (under my local ubuntu 22.04): # build-sanitizer
make -j$(nproc) TARGET=${{env.TARGET }} DEBUG=${{ env.DEBUG }} VFB=y SANITIZE=y DEBUG_GLIBCXX=y USE_CCACHE=y V=2 everything check
make -j$(nproc) TARGET=UNIX DEBUG=y VFB=y SANITIZE=y DEBUG_GLIBCXX=y USE_CCACHE=y V=2 everything check
# after adding `$(SRC)/Gauge/NavigatorSettings.cpp \`` in test.mk line 1693 and line 2121,
# the build-sanitizer (above make command) process reach the end and the tests passed successfully locally (!)
# build-ubuntu
make -j$(nproc) TARGET=${{env.TARGET }} DEBUG=${{ env.DEBUG }} USE_CCACHE=y V=2 everything check
make -j$(nproc) TARGET=UNIX DEBUG=y USE_CCACHE=y V=2 everything check
# the modifications done for build-sanitizer allows the build-ubuntu (above make command) process
# to reach the end and the tests also passed successfully locally (!)The modifications in test.mk "$(SRC)/Gauge/NavigatorSettings.cpp " added in lines 1693 and 2121 allow to pass successfully locally the 2 build processes (build-ubuntu and build-sanitizer) and their related tests. |
FEATURES:
+ MAIN FRAME FOR INFORMATIONS ABOUT CURRENT TASK
- average task speed
- Icon of previous Waypoint
- Progress bar / time informations for the current task
- time start
- current time (current task duration)
- estimated time end (estimated task duration)
+ INCLUDED FRAME FOR NEXT WAYPOINT INFORMATIONS
- Name
- Distance
- Altitude (Next altitude arrival): Absolute arrival altitude at the next waypoint in final glide
- Next GR (required glide ration over ground to reach the best waypoint)
- Arrow Icon: bearing direction to the next waypoint
- North Ring Icon: bearing direction to the north
- information added: current speed, current altitude
- Icon of next Waypoint
+ FUNCTIONNALITIES
- manage different task types:
- TaskType::ORDERED.
- TaskType::GOTO
- TaskType::ABORT
- TaskType::NONE
- switch to alternate when there is no task or when the task is suspended (Nav->Task Abort)
- all texts scale according to size of the top widget. if the width of the bar is too small, information disappear accordingly.
- gesture in Navigator Widget:
- <-- Left: previous Waypoint
- --> Right: next Waypoint
- --> <-- Right, Left: Next Target
- <-- --> Left, Right: Alternates
- Double click: Standard menu
- Quick click (400ms < press < 1000ms): Analysis task
- Long click (1000ms < press < 3000ms): Task manager
- units (small size) beside data as in infoboxes
(km/h, mp/h, knots, m, ft, ...) according to user unit settings
- data placement in regards to the widget size
+ CONFIGURATION
- save configuration in profile file
- units in frames follow the user setup:
Config->System->Setup->Units
(european, american, australian, british units, ...)
- inversed colors update automatically with user setup ; aka dark / light mode
(inverse Infobox setting in the screen layout menu)
Config->System->Look->Screen Layout->Inverse InfoBoxes
- setup height of the navigator widget (default height set to 17%)
Config->System->Gauges->FLARM, Other
threaderic
force-pushed
the
NavigationWidget
branch
from
5db755e
to
a5e01a6
Compare
Brief summary of the changes
add a new feature --> Navigator widget above the map
The aim of this new widget is to provide the user a summary of informations about the current task / next waypoint at the same place.
Brief description / features
Features:
Main frame for informations about current task
Included Frame for Next Waypoint informations
functionnalities
(km/h, mp/h, knots, m, ft, ...) according to user unit settings
configuration
Config->System->Setup->Units
(european, american, australian, british units, ...)
(inverse Infobox setting in the screen layout menu)
Config->System->Look->Screen Layout->Inverse InfoBoxes
Config->System->Gauges->FLARM, Other
Differents pages setting
all features
Differents hardware screens (eg. smartphone, e-reader), dark / light modes
Focus on Bearing Arrow / Ring North direction icons
Gestures
Pages setting
Navigator's height setting
Related issues and discussions
fixed (?) issues in XCSoar
That cause bad heights of both widgets.
see PageActions.cpp
TODO:
IDEAS for additional features: