GoldenCheetah
Rebrand Diary View to Plan View
To support the new planning chart(s) the Diary View has been updated to Plan View.
This is an initial start to the full Plan View, the following have been updated:
- Class name DiaryView -> PlanView
- Class name DiarySidebar -> PlanSideBar
- Added "Plan" button to Edit Menu
- Diary configuration changed to Plan configuration names
- New plan-perspective.xml configuration file
- New default "baked in" plan-perspective.xml configuration file with single PlanningCalendarWindow chart
- "Legacy" perspective name has been restricted to be applied only for the old -layout.xml files
- Tested PlanningCalendarWindow export and import
- Tested NavigationModel, see notes below
The following have been noticed or need to be addressed:
i) I have assumed that re-using the old diary 0x04 view type src/Gui/GcWindowRegistry.h() is ok ?
ii) The navigation undo actions can cause a crash! After changing the timeframe in the trends view, which then caused the context->currentSeason() to return nullptr. I have added protection for this, please search for PLAN_VIEW_TODO
iii) Should the PlanView and TrendsView have independent date ranges (search for PLAN_VIEW_TODO) ? At the moment they use the same one, which results in some interesting behaviour, for example, if dates only in 2024 are set in trends, when switching to the Plan View will not allow the new planning calendar to display dates in 2025, it is restricted to 2024 dates. Now the planning chart is in its own view rather than part of the trends view, it may need its only independent date range.
Please let me know if anything is wrong, could be improved or needs to be update prior to merging
Thanks, Paul, it looks ok for me.
The following have been noticed or need to be addressed:
i) I have assumed that re-using the old diary 0x04 view type src/Gui/GcWindowRegistry.h() is ok ?
Yes, but for now I would let all charts enabled for DIARY_VIEW enabled for PLAN_VIEW, besides the new Calendar chart.
ii) The navigation undo actions can cause a crash! After changing the timeframe in the trends view, which then caused the context->currentSeason() to return nullptr. I have added protection for this, please search for PLAN_VIEW_TODO
Right, usually there is a Season/DateRange (and a Ride) selected, but the code should not crash when they are not.
iii) Should the PlanView and TrendsView have independent date ranges (search for PLAN_VIEW_TODO) ? At the moment they use the same one, which results in some interesting behaviour, for example, if dates only in 2024 are set in trends, when switching to the Plan View will not allow the new planning calendar to display dates in 2025, it is restricted to 2024 dates. Now the planning chart is in its own view rather than part of the trends view, it may need its only independent date range.
No, I don't think so, that would require extensive changes to the APIs (DataFilter/R/Python). Lets think about other solution for this.
Thanks, Paul, it looks ok for me.
PR updated with the changes you suggested:
The following have been noticed or need to be addressed: i) I have assumed that re-using the old diary 0x04 view type src/Gui/GcWindowRegistry.h() is ok ?
Yes, but for now I would let all charts enabled for DIARY_VIEW enabled for PLAN_VIEW, besides the new Calendar chart.
I have re-enabled all the original Diary charts to be available under Plan view. I have also made the new Calendar chart available in both trends and plan, otherwise I'd also need to update the baked in plan perspective I have created. This will allow testing of the new Calendar chart, as we work out the issues of moving it from trends to plan. (Let me know if the new Calendar chart must only be in Diary view)
ii) The navigation undo actions can cause a crash! After changing the timeframe in the trends view, which then caused the context->currentSeason() to return nullptr. I have added protection for this, please search for PLAN_VIEW_TODO
Right, usually there is a Season/DateRange (and a Ride) selected, but the code should not crash when they are not.
I have added protection for this, and removed the todo comment.
iii) Should the PlanView and TrendsView have independent date ranges (search for PLAN_VIEW_TODO) ? At the moment they use the same one, which results in some interesting behaviour, for example, if dates only in 2024 are set in trends, when switching to the Plan View will not allow the new planning calendar to display dates in 2025, it is restricted to 2024 dates. Now the planning chart is in its own view rather than part of the trends view, it may need its only independent date range.
No, I don't think so, that would require extensive changes to the APIs (DataFilter/R/Python). Lets think about other solution for this.
I was just trying to raise the issue which I don't have enough knowledge to resolve, I have removed the todo comment.
I have updated this PR to correct the new code (VIEW_DIARY to VIEW_PLAN) added by #4698, and to correct the perspective dialog's horizontal title bar which appeared indented when in Plan view with only a single table entry.
Hi Paul, I marked this as WIP since we need to find a solution for the sidebar, the current one is not useful and the Calendar depending on Season/Phase selected in other view is not practical.
@amtriathlon Regarding the trends and planning sidebar do you want me to look at a solution where both trends and planning have a graphical instance, but share the same model? So any date/filtering in trends (or planning) would apply to both views, and the sidebar would appear the same (unchanged) when switching views?
Yes, that could be a viable solution, both views need Season and Events.
Hi @amtriathlon please the latest commits.
I have looked at a number of solutions for sharing the LTMSidebar between the Trends & Plan view. I tried extracting and making a common model for the two separate views sidebars, but this creates a huge model trying to capture all the items selections and sizes to keep them aligned. I also looked at using a QGraphicsView class to provide multiple views (for trends & plan) onto a single QGraphicsScene containing the LTMSidebar, but this wasn't going to work.
So the solution for the Long Term Metrics (LTM) sidebar for the Trends & Plan view, is for them to share the same LTM instance, and on a viewChange() event detach the sidebar from the old view and re-attach it to the new view. This ensures they have same look & feel and all the settings are the same. To achieve this I have added an LTMSidebarView class that both Trends & Plan view inherit from, and this class manages the switching of the LTMSidebar. It also ensures there is one LTMSidebar per athlete/context, so each athlete's settings will be preserved when changing between athletes.
I have corrected the startupView flag, see #4735.
The changes:
Views.[h|cpp] Added new LTMSidebarView class Move duplicated Trends and Plan view code to the inherited LTMSidebarView class Removed the unused setRide() function from Plan view, I'm not sure why this was in the original Diary class
AbstractView[h|cpp] Modified the setSidebar() function to allow sidebars to be detached.
PlanSidebar.[h|cpp] I have removed the PlanSidebar class from the PlanSidebar.[h|cpp] and renamed the files to MiniCalendar.[h|cpp] to better reflect the remaining contents
Settings[h|cpp] Added a default sideplan attribute, to match those for the other views
LTMSidebar.[h|cpp] Remove the unused from and to attributes.
NavigationModel.cpp Updated and simplified the LTMSidebar access
RideNavigator.h Removed the unused pre-declaration of PlanSidebar class
@amtriathlon Please see the latest commit which hides the LTMSidebar preset charts on the planning view
Hi Paul, chart library hide/show is ok. OTOH, I found Metric Trends and User Charts don't respond to Date Range changes when they are in Plan View.
I'll take a look at the Metric Trends and User Charts don't respond to Date Range changes when they are in Plan View
@amtriathlon Please see latest commit for a fix for the Metric Trends and User Charts don't respond to Date Range changes when they are in Plan View: https://github.com/GoldenCheetah/GoldenCheetah/pull/4690/commits/4d61707f4055ad36295f2a0a55dff4f757a60b57
@amtriathlon I have tidied the old diary/plan sidebar configuration and removed the trends/plan view justSelected() duplication.
But I need your opinion on the commit https://github.com/GoldenCheetah/GoldenCheetah/pull/4690/commits/9da7fe5184addb182ffa9300144abe56b92a2bf6, as I have extended the plan view changes to the overview charts and overview scope, which will support any differences/divergence of the plan view from the trends view in the future. Please let me know if this needs to be changed, or this commit reverted?
Hi Paul, this seems to be working as expected, there are some warnings at the console when running with --debug:
QSplitter::replaceWidget: Widget can't be null (unknown:0) Triggered by src/Gui/AbstractView.cpp:186 - splitter->replaceWidget(0, nullptr)
Additional trigger / call of this method with nullptr: void LTMSidebarView::hideEvent(QHideEvent*) { setSidebar(nullptr); }
which should be fixed before merging.
@amtriathlon Please see latest push which fixes the splitter->replaceWidget(0, nullptr) issue, and also ensures only views can set the sidebars (as setting a null sidebar should only be done by the views), and I have updated the default configuration so that the calendar & agenda charts are the defaults for plan view.