Move place info to panel/dock #8259

Open
dmfrodrigues wants to merge 1 commit from dmfrodrigues/place-panel into master
dmfrodrigues commented 2024-05-23 12:15:29 +00:00 (Migrated from github.com)

Moves place page dialog to a panel on the left.

This makes the place information less obstructive, allowing the user to navigate the map even when a place is selected. This diff makes the desktop version more consistent with the mobile version, where the place info is shown in a panel, and does not impede interaction with the map.

User mode

Before image
After image

Developer mode

Before image
After image

My two main issues

  1. This is a significant UI change, and it is pretty opinionated. So if you have any suggestions or oppose this change, please speak up 🙂
  2. I am unsure on the best way to implement the user vs developer interface. The previous design with two different classes, one for PlacePageDialogUser and another for PlacePageDialogDeveloper, was fine when we were creating the PlacePageDialog on the fly. But now that the PlacePanel is somewhat of a more persistent object in the UI (since it is now in the side panel), it is more cumbersome to create new Panel instances on the fly.
    • I've already tried getting the MainWindow to tell DrawWidget that developer mode was changed, and then DrawWidget would change the type of PlacePanel it was displaying, but it was pretty cumbersome (besides, the PlacePanel is being injected into the DrawWidget by the MainWindow, since the PlacePanel needs to be created by the MainWindow so that it ends up in the correct Dock).
    • Another possible solution would be to move all the User and Developer logic to the same class, PlacePanel, and depending on the value of DeveloperMode, PlacePanel::setPlace would add the correct elements. I like this solution, but this implies getting rid of PlacePageDialogUser and PlacePageDialogDeveloper. What do you think @Ferenc- ? (I'm asking you since you suggested this split in #7505)
Moves place page dialog to a panel on the left. This makes the place information less obstructive, allowing the user to navigate the map even when a place is selected. This diff makes the desktop version more consistent with the mobile version, where the place info is shown in a panel, and does not impede interaction with the map. ### User mode | | | |-|-| | Before | ![image](https://github.com/organicmaps/organicmaps/assets/43684166/acbb1b8e-8b47-4595-b1ab-7662da7f9fab) | | After | ![image](https://github.com/organicmaps/organicmaps/assets/43684166/98117906-17d5-43f1-8f6e-5324c69a0b2e) | ### Developer mode | | | |-|-| | Before | ![image](https://github.com/organicmaps/organicmaps/assets/43684166/572c0182-df75-410e-b81e-b4ead37535e1) | | After | ![image](https://github.com/organicmaps/organicmaps/assets/43684166/a77709a1-1a7c-4587-8fba-e2539448bcbd) | ### My two main issues 1. This is a significant UI change, and it is pretty opinionated. So if you have any suggestions or oppose this change, please speak up :slightly_smiling_face: 2. I am unsure on the best way to implement the user vs developer interface. The previous design with two different classes, one for PlacePageDialogUser and another for PlacePageDialogDeveloper, was fine when we were creating the PlacePageDialog on the fly. But now that the PlacePanel is somewhat of a more persistent object in the UI (since it is now in the side panel), it is more cumbersome to create new Panel instances on the fly. - I've already tried getting the MainWindow to tell DrawWidget that developer mode was changed, and then DrawWidget would change the type of PlacePanel it was displaying, but it was pretty cumbersome (besides, the PlacePanel is being injected into the DrawWidget by the MainWindow, since the PlacePanel needs to be created by the MainWindow so that it ends up in the correct Dock). - Another possible solution would be to move all the User and Developer logic to the same class, PlacePanel, and depending on the value of DeveloperMode, PlacePanel::setPlace would add the correct elements. I like this solution, but this implies getting rid of PlacePageDialogUser and PlacePageDialogDeveloper. What do you think @Ferenc- ? (I'm asking you since you suggested this split in #7505)
vng (Migrated from github.com) reviewed 2024-05-23 12:15:29 +00:00
biodranik (Migrated from github.com) reviewed 2024-05-23 12:15:29 +00:00
vng (Migrated from github.com) requested changes 2024-06-05 02:31:33 +00:00
vng (Migrated from github.com) commented 2024-06-05 02:23:09 +00:00

nit: Qt code style assumes QWidget * parent in the end.

nit: Qt code style assumes ```QWidget * parent``` in the end.
vng (Migrated from github.com) commented 2024-06-05 02:23:48 +00:00

nit: Remove blank lines.

nit: Remove blank lines.
@ -636,3 +640,3 @@
}
void DrawWidget::ShowPlacePage()
void DrawWidget::UpdatePlace()
vng (Migrated from github.com) commented 2024-06-05 02:24:42 +00:00

nit: We have space in our code-style if (...)

nit: We have space in our code-style ```if (...)```
@ -0,0 +53,4 @@
public:
QHLine(QWidget * parent = nullptr) : QFrame(parent)
{
setFrameShape(QFrame::HLine);
vng (Migrated from github.com) commented 2024-06-05 02:26:09 +00:00

Something strange here. Did you mean?

if (auto const pLayout = layout())
  QWidget::setLayout(pLayout);

Otherwise QWidget().setLayout looks like setting something for the temporary object.

Something strange here. Did you mean? ``` if (auto const pLayout = layout()) QWidget::setLayout(pLayout); ``` Otherwise QWidget().setLayout looks like setting something for the temporary object.
vng (Migrated from github.com) commented 2024-06-05 02:31:16 +00:00

Well, have some doubts here. Since the dock panel is persistent, we can store a pointer on a temporary object here and get some UB.

Well, have some doubts here. Since the dock panel is persistent, we can store a pointer on a temporary object here and get some UB.
@ -0,0 +312,4 @@
{
QHLine * line = new QHLine();
innerLayout->addWidget(line);
}
vng (Migrated from github.com) commented 2024-06-05 02:28:50 +00:00

Same here

Same here
vng (Migrated from github.com) commented 2024-06-05 02:29:25 +00:00

nit: m_infoPtr

nit: m_infoPtr
dmfrodrigues (Migrated from github.com) reviewed 2024-06-05 12:13:33 +00:00
@ -0,0 +53,4 @@
public:
QHLine(QWidget * parent = nullptr) : QFrame(parent)
{
setFrameShape(QFrame::HLine);
dmfrodrigues (Migrated from github.com) commented 2024-06-05 12:13:32 +00:00

Yeah it's a bit strange, but it is apparently a common solution to delete a layout. If you set a layout of a temporary Widget, when the widget is deleted so is the layout. And that's what I'm doing here, since it is easier to just delete and re-create the layout when the place changes.

The alternative solution is to call delete this->layout(), but this does not delete sub-layouts or children widgets, as pointed out here.

Yeah it's a bit strange, but it is apparently a common solution to delete a layout. If you set a layout of a temporary Widget, when the widget is deleted so is the layout. And that's what I'm doing here, since it is easier to just delete and re-create the layout when the place changes. The alternative solution is to call `delete this->layout()`, but this does not delete sub-layouts or children widgets, as pointed out [here](https://stackoverflow.com/questions/7528680/how-to-delete-an-already-existing-layout-on-a-widget).
dmfrodrigues (Migrated from github.com) reviewed 2024-06-05 12:35:54 +00:00
@ -0,0 +53,4 @@
public:
QHLine(QWidget * parent = nullptr) : QFrame(parent)
{
setFrameShape(QFrame::HLine);
dmfrodrigues (Migrated from github.com) commented 2024-06-05 12:35:54 +00:00

Removed this part.

Removed this part.
dmfrodrigues (Migrated from github.com) reviewed 2024-06-05 12:38:19 +00:00
@ -0,0 +53,4 @@
public:
QHLine(QWidget * parent = nullptr) : QFrame(parent)
{
setFrameShape(QFrame::HLine);
dmfrodrigues (Migrated from github.com) commented 2024-06-05 12:38:18 +00:00

@vng do you think this could lead to undefined behavior? Because we're capturing info by reference to this lambda that will run at a later point, and I'm not sure if info is deleted after setPlace is called. Is capturing info by copy an option (i.e., can place_page::Info be copied)?

@vng do you think this could lead to undefined behavior? Because we're capturing `info` by reference to this lambda that will run at a later point, and I'm not sure if `info` is deleted after `setPlace` is called. Is capturing `info` by copy an option (i.e., can `place_page::Info` be copied)?
vng (Migrated from github.com) reviewed 2024-06-05 12:40:17 +00:00
@ -0,0 +53,4 @@
public:
QHLine(QWidget * parent = nullptr) : QFrame(parent)
{
setFrameShape(QFrame::HLine);
vng (Migrated from github.com) commented 2024-06-05 12:40:17 +00:00

Please, take it out as a separate function DeleteExistingLayout with a comment to SOF link.

Please, take it out as a separate function DeleteExistingLayout with a comment to SOF link.
vng (Migrated from github.com) reviewed 2024-06-05 12:45:07 +00:00
@ -0,0 +53,4 @@
public:
QHLine(QWidget * parent = nullptr) : QFrame(parent)
{
setFrameShape(QFrame::HLine);
vng (Migrated from github.com) commented 2024-06-05 12:45:07 +00:00

It should be fine here.
Framework::GetCurrentPlacePageInfo() returns persistent object.
But the state can change when invoking lambda.

It should be fine here. ```Framework::GetCurrentPlacePageInfo()``` returns persistent object. But the state can change when invoking lambda.
dmfrodrigues (Migrated from github.com) reviewed 2024-06-05 13:51:06 +00:00
@ -0,0 +53,4 @@
public:
QHLine(QWidget * parent = nullptr) : QFrame(parent)
{
setFrameShape(QFrame::HLine);
dmfrodrigues (Migrated from github.com) commented 2024-06-05 13:51:06 +00:00

Hmm then it's probably better to capture by copy.

Hmm then it's probably better to capture by copy.
vng (Migrated from github.com) reviewed 2024-06-05 14:26:10 +00:00
@ -0,0 +53,4 @@
public:
QHLine(QWidget * parent = nullptr) : QFrame(parent)
{
setFrameShape(QFrame::HLine);
vng (Migrated from github.com) commented 2024-06-05 14:24:22 +00:00

Put this check inside DeleteExistingLayout

Put this check inside DeleteExistingLayout
@ -0,0 +182,4 @@
{
QLabel * name = new QLabel("Wikipedia");
name->setOpenExternalLinks(true);
name->setTextInteractionFlags(Qt::TextBrowserInteraction);
vng (Migrated from github.com) commented 2024-06-05 14:25:39 +00:00

What is wrong with auto const &?

What is wrong with ```auto const &```?
@ -0,0 +310,4 @@
innerLayout->addStretch();
{
QHLine * line = new QHLine();
vng (Migrated from github.com) commented 2024-06-05 14:26:00 +00:00

{ from new line

{ from new line
biodranik (Migrated from github.com) reviewed 2024-06-07 06:18:50 +00:00
biodranik (Migrated from github.com) left a comment

Thanks! Here is the testing feedback:

  1. Some lines are collapsed without any way to see their content if necessary:
    выява

Is it possible to add some indication (...), or show the full string in the hover title, or some other ideas?

  1. There is annoying trembling when the panel is opened, and the map is clicked, see video:
    https://github.com/organicmaps/organicmaps/assets/170263/83cffa2f-8f17-47ac-ac06-4043ea4bc215

  2. The escape key does not close the panel.

  3. Unrelated minor issue: when the search panel is closed by the "x" button, the search button on the right button bar stays selected.

Thanks! Here is the testing feedback: 1. Some lines are collapsed without any way to see their content if necessary: <img width="379" alt="выява" src="https://github.com/organicmaps/organicmaps/assets/170263/b57454db-2115-4ef6-818c-9e2f6204dc9c"> Is it possible to add some indication (...), or show the full string in the hover title, or some other ideas? 2. There is annoying trembling when the panel is opened, and the map is clicked, see video: https://github.com/organicmaps/organicmaps/assets/170263/83cffa2f-8f17-47ac-ac06-4043ea4bc215 3. The escape key does not close the panel. 4. Unrelated minor issue: when the search panel is closed by the "x" button, the search button on the right button bar stays selected.
@ -635,8 +639,11 @@ void DrawWidget::OnRouteRecommendation(RoutingManager::Recommendation recommenda
}
}
biodranik (Migrated from github.com) commented 2024-06-07 06:08:55 +00:00

Why lowerCase?

void DrawWidget::UpdatePlace()
Why lowerCase? ```suggestion void DrawWidget::UpdatePlace() ```
biodranik (Migrated from github.com) commented 2024-06-07 06:10:12 +00:00
void MainWindow::CreatePlaceBarAndPanel()
{
```suggestion void MainWindow::CreatePlaceBarAndPanel() { ```
biodranik (Migrated from github.com) commented 2024-06-07 06:13:17 +00:00
  CreatePanelImpl(1, Qt::LeftDockWidgetArea, tr("Place Information Page"), QKeySequence(), 0);
```suggestion CreatePanelImpl(1, Qt::LeftDockWidgetArea, tr("Place Information Page"), QKeySequence(), 0); ```
@ -0,0 +7,4 @@
#include "platform/settings.hpp"
#include <QtWidgets/QDialogButtonBox>
#include <QtWidgets/QScrollArea>
biodranik (Migrated from github.com) commented 2024-06-07 06:13:48 +00:00

constexpr here and below, it is not clear what's the dimension of the variable (pixels?)

constexpr here and below, it is not clear what's the dimension of the variable (pixels?)
@ -0,0 +53,4 @@
public:
QHLine(QWidget * parent = nullptr) : QFrame(parent)
{
setFrameShape(QFrame::HLine);
biodranik (Migrated from github.com) commented 2024-06-07 06:15:30 +00:00

Does it fit 120 chars limit from our guideline?

void PlacePanel::setPlace(place_page::Info const & info, search::ReverseGeocoder::Address const & address)
Does it fit 120 chars limit from our guideline? ```suggestion void PlacePanel::setPlace(place_page::Info const & info, search::ReverseGeocoder::Address const & address) ```
biodranik (Migrated from github.com) commented 2024-06-07 06:15:49 +00:00

ditto 120

ditto 120
@ -0,0 +308,4 @@
}
innerLayout->addStretch();
biodranik (Migrated from github.com) commented 2024-06-07 06:15:59 +00:00

ditto 120

ditto 120
biodranik (Migrated from github.com) commented 2024-06-07 06:17:52 +00:00
  if (this->layout() != nullptr)
  {
```suggestion if (this->layout() != nullptr) { ```
biodranik (Migrated from github.com) commented 2024-06-07 06:18:12 +00:00
enum PressedButton : int 
{
```suggestion enum PressedButton : int { ```
biodranik (Migrated from github.com) commented 2024-06-07 06:18:21 +00:00

ditto 120

ditto 120
biodranik (Migrated from github.com) commented 2024-06-07 06:18:30 +00:00

ditto 120

ditto 120
biodranik (Migrated from github.com) commented 2024-06-07 06:18:36 +00:00

ditto 120

ditto 120
Ferenc- (Migrated from github.com) approved these changes 2024-06-08 17:51:11 +00:00
This repo is archived. You cannot comment on pull requests.
No labels
Accessibility
Accessibility
Address
Address
Android
Android
Android Auto
Android Auto
Android Automotive (AAOS)
Android Automotive (AAOS)
API
API
AppGallery
AppGallery
AppStore
AppStore
Battery and Performance
Battery and Performance
Blocker
Blocker
Bookmarks and Tracks
Bookmarks and Tracks
Borders
Borders
Bug
Bug
Build
Build
CarPlay
CarPlay
Classificator
Classificator
Community
Community
Core
Core
CrashReports
CrashReports
Cycling
Cycling
Desktop
Desktop
DevEx
DevEx
DevOps
DevOps
dev_sandbox
dev_sandbox
Directions
Directions
Documentation
Documentation
Downloader
Downloader
Drape
Drape
Driving
Driving
Duplicate
Duplicate
Editor
Editor
Elevation
Elevation
Enhancement
Enhancement
Epic
Epic
External Map Datasets
External Map Datasets
F-Droid
F-Droid
Fonts
Fonts
Frequently User Reported
Frequently User Reported
Fund
Fund
Generator
Generator
Good first issue
Good first issue
Google Play
Google Play
GPS
GPS
GSoC
GSoC
iCloud
iCloud
Icons
Icons
iOS
iOS
Legal
Legal
Linux Desktop
Linux Desktop
Linux packaging
Linux packaging
Linux Phone
Linux Phone
Mac OS
Mac OS
Map Data
Map Data
Metro
Metro
Navigation
Navigation
Need Feedback
Need Feedback
Night Mode
Night Mode
NLnet 2024-06-281
NLnet 2024-06-281
No Feature Parity
No Feature Parity
Opening Hours
Opening Hours
Outdoors
Outdoors
POI Info
POI Info
Privacy
Privacy
Public Transport
Public Transport
Raw Idea
Raw Idea
Refactoring
Refactoring
Regional
Regional
Regression
Regression
Releases
Releases
RoboTest
RoboTest
Route Planning
Route Planning
Routing
Routing
Ruler
Ruler
Search
Search
Security
Security
Styles
Styles
Tests
Tests
Track Recording
Track Recording
Translations
Translations
TTS
TTS
UI
UI
UX
UX
Walk Navigation
Walk Navigation
Watches
Watches
Web
Web
Wikipedia
Wikipedia
Windows
Windows
Won't fix
Won't fix
World Map
World Map
No milestone
No project
No assignees
1 participant
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: organicmaps/organicmaps-tmp#8259
No description provided.