MapEditorController: Refactor map notes window by dl3sdo · Pull Request #2272 · OpenOrienteering/mapper

dl3sdo · 2024-08-05T19:24:54Z

The map notes dialog was quite small and did not automatically adjust itself to the size of the map notes upon creation. This commit defines a minimum and maximum window size and adjusts the size wthin these limits according to the size of the map notes.

Closes#2119

dl3sdo · 2024-08-05T19:40:17Z

Minimum size (appearance after opening dialog):

Maximum size (appearance after opening dialog):

dg0yt · 2024-11-25T07:26:01Z

IMO this dialog shouldn't be maintainted of the map_editor.cpp monster.

dl3sdo · 2024-11-25T07:46:11Z

I agree, and I will move it to a separate file.

The map notes dialog was quite small and did not automatically adjust itself to the size of the map notes upon creation. This commit defines a minimum and maximum window size and adjusts the size wthin these limits according to the size of the map notes.

The map notes dialog had a non-standard button layout (with the Cancel button on the left side and the OK button on the right side). In addition both buttons were non-standard buttons (the OK button contained a green right arrow as icon). Change the map notes dialog layout to use standard buttons as for all other dialogs in Mapper.

dg0yt

Do we want to guard against map == nullptr?

dg0yt · 2024-11-26T05:59:16Z

+: QDialog(parent, Qt::WindowSystemMenuHint | Qt::WindowTitleHint | Qt::WindowCloseButtonHint)
+, map { map }
+{
+	setWindowTitle(QCoreApplication::translate("OpenOrienteering::MapNotes", "Map notes"));


Either
Suggested change
setWindowTitle(QCoreApplication::translate("OpenOrienteering::MapNotes", "Map notes"));
setWindowTitle(QCoreApplication::translate("OpenOrienteering::MapEditorController", "Map notes"));
or
Suggested change
setWindowTitle(QCoreApplication::translate("OpenOrienteering::MapNotes", "Map notes"));
setWindowTitle(tr("Map notes"));

Choosing setWindowTitle(tr("Map notes"));

Do we want to guard against map == nullptr?
I think that it's not possible to select this menu item without having a valid map. At least scaleMapClicked() does not check it neither and RotateMapDialog even does dereference the pointer.

Okay... You are the key user for crashing other people's software.

Easy check OTOH.

dg0yt · 2024-11-26T06:04:06Z

+	width = qMax(300, qMin(width, bounding_rect.width() + 60));
+	height = qMax(200, qMin(height, bounding_rect.height() + 80));


Suggested change
width = qMax(300, qMin(width, bounding_rect.width() + 60));
height = qMax(200, qMin(height, bounding_rect.height() + 80));
width = qBound(300, width, bounding_rect.width() + 60);
height = qBound(200, height, bounding_rect.height() + 80);

PappyEric · 2024-11-26T07:04:55Z

Sooo seriously, when are we going to get a release that doesn't take a technical degree to install??? Keep making excuses, and keep turning away supporters. Sent from Proton Mail Android

…

-------- Original Message --------

On 11/26/24 1:08 AM, Kai Pastor wrote: @dg0yt commented on this pull request. Do we want to guard against map == nullptr? --------------------------------------------------------------- In [src/gui/map/map_notes.cpp](#2272 (comment)): > +#include <QScreen> +#include <QSize> +#include <QTextEdit> +#include <QVBoxLayout> +#include <QWidget> + +#include "core/map.h" + + +namespace OpenOrienteering { + +MapNotesDialog::MapNotesDialog(QWidget* parent, Map* map) +: QDialog(parent, Qt::WindowSystemMenuHint | Qt::WindowTitleHint | Qt::WindowCloseButtonHint) +, map { map } +{ + setWindowTitle(QCoreApplication::translate("OpenOrienteering::MapNotes", "Map notes")); Either ⬇️ Suggested change - setWindowTitle(QCoreApplication::translate("OpenOrienteering::MapNotes", "Map notes")); + setWindowTitle(QCoreApplication::translate("OpenOrienteering::MapEditorController", "Map notes")); or ⬇️ Suggested change - setWindowTitle(QCoreApplication::translate("OpenOrienteering::MapNotes", "Map notes")); + setWindowTitle(tr("Map notes")); --------------------------------------------------------------- In [src/gui/map/map_notes.cpp](#2272 (comment)): > + width = qMax(300, qMin(width, bounding_rect.width() + 60)); + height = qMax(200, qMin(height, bounding_rect.height() + 80)); ⬇️ Suggested change - width = qMax(300, qMin(width, bounding_rect.width() + 60)); - height = qMax(200, qMin(height, bounding_rect.height() + 80)); + width = qBound(300, width, bounding_rect.width() + 60); + height = qBound(200, height, bounding_rect.height() + 80); — Reply to this email directly, [view it on GitHub](#2272 (review)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AEKV6JXA6E5WOKDVUTBZAH32CQF4NAVCNFSM6AAAAABMA4OTO2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINRQGQZDSMJUHE). You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

dg0yt · 2024-11-26T07:06:27Z

Take a technical degree to install???

Be careful. Humans on both sides.

PappyEric · 2024-11-26T07:19:20Z

Yes, I've tried to follow the documentation to install openorienteering a few times and failed. Is the documentation out of date? I only see the same few people create commits here. This doesn't seem like an Open architecture since it's been several, several years since updating the main core. Sent from Proton Mail Android

…

-------- Original Message --------

On 11/26/24 2:06 AM, Kai Pastor wrote: > Take a technical degree to install??? Be careful. Humans on both sides. — Reply to this email directly, [view it on GitHub](#2272 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AEKV6JXP4IGCQHUNGIRNC7L2CQMYTAVCNFSM6AAAAABMA4OTO2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOJZHAZDCNRRHA). You are receiving this because you commented.Message ID: ***@***.***>

PappyEric · 2024-11-26T07:34:53Z

Let me reiterate, I've tried to install the GIT version of Openorienteering so I could maybe add some input. Tried, and tried again. From an outsider it seems like there's just a select few of people that can contribute. Just make a new release ... Sent from Proton Mail Android

…

-------- Original Message --------

On 11/26/24 2:19 AM, wrote: Yes, I've tried to follow the documentation to install openorienteering a few times and failed. Is the documentation out of date? I only see the same few people create commits here. This doesn't seem like an Open architecture since it's been several, several years since updating the main core. Sent from Proton Mail Android -------- Original Message -------- On 11/26/24 2:06 AM, Kai Pastor wrote: >> Take a technical degree to install??? > > Be careful. Humans on both sides. > > — > Reply to this email directly, [view it on GitHub](#2272 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AEKV6JXP4IGCQHUNGIRNC7L2CQMYTAVCNFSM6AAAAABMA4OTO2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOJZHAZDCNRRHA). > You are receiving this because you commented.Message ID: ***@***.***>

dg0yt · 2024-11-26T07:39:44Z

Let me reiterate, I've tried to install the GIT version of Openorienteering so I could maybe add some input. Tried, and tried again.

Let me reiterate:
If you want to ask for help with building from source, open an issue or discussion.
If you want to not slow down things, kindly avoid rants on unrelated issues and PRs.

PappyEric · 2024-11-26T07:46:29Z

You're slowing things down. Sent from Proton Mail Android

…

-------- Original Message --------

On 11/26/24 2:40 AM, Kai Pastor wrote: > Let me reiterate, I've tried to install the GIT version of Openorienteering so I could maybe add some input. Tried, and tried again. Let me reiterate: If you want to ask for help with building from source, open an issue or discussion. If you want to not slow down things, kindly avoid rants on unrelated issues and PRs. — Reply to this email directly, [view it on GitHub](#2272 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AEKV6JWBRGU3H5QLB2NCI732CQQVLAVCNFSM6AAAAABMA4OTO2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOJZHA4DINZYGQ). You are receiving this because you commented.Message ID: ***@***.***>

dg0yt · 2024-11-26T07:48:24Z

@PappyEric Sorry, I have to put you on my block list. Toxic discussion are not beneficial.

lpechacek · 2024-11-26T08:00:06Z

@PappyEric No matter how much I understand your position and why you are discussing your issue right here, please mind that discussion is off-topic in the context of this code pull request. The Discussions place might be a better home for conversations about the project leadership, perhaps with a more articulated set of reproaches and honors towards the OO developers team. Thanks!

PappyEric · 2024-11-26T08:03:02Z

@PappyEric No matter how much I understand your position and why you are discussing your issue right here, please mind that discussion is off-topic in the context of this code pull request. The Discussions place might be a better home for conversations about the project leadership, perhaps with a more articulated set of reproaches and honors towards the OO developers team. Thanks!

Sorry

dl3sdo · 2024-11-26T16:01:52Z

Shall I keep three commits (I would split the 4th one and squash appropriately) or shall I squash and update the commit message?

dg0yt · 2024-11-26T19:08:17Z

I would squash the PR with the merge. Unless you want control over the commit message 😄

Consider app window size instead of screen size. Consider max line length for better readability.

dg0yt · 2024-11-29T04:40:51Z

FTR I want to limit width because long lines are hard to read.

dg0yt · 2024-11-29T04:41:41Z

/azp run

azure-pipelines · 2024-11-29T04:41:54Z

Azure Pipelines successfully started running 1 pipeline(s).

dg0yt · 2024-11-29T06:56:37Z

I should really fix the flaky hdiutil job at its root 😠

Moved out of MapEditorController. Improved window size calculation using available space while maintaining a readable max width. Use standard buttons and layout for map notes dialog. Co-authored-by: Kai Pastor <dg0yt@darc.de>

dl3sdo force-pushed the increase-map-notes branch from f7467aa to dbc49f4Compare August 5, 2024 20:03

lpechacek self-requested a review August 6, 2024 06:22

dl3sdo marked this pull request as draft November 25, 2024 07:46

dl3sdo changed the title ~~MapEditorController: Increase size of map notes window~~MapEditorController: Refactor map notes windowNov 25, 2024

dl3sdo added 3 commits November 25, 2024 22:54

MapEditorController: Move map notes dialog to new files
d0a2289

dl3sdo force-pushed the increase-map-notes branch from 42310c5 to d0a2289Compare November 25, 2024 21:55

dg0yt reviewed Nov 26, 2024
View reviewed changes

lpechacek mentioned this pull request Nov 26, 2024
AboutDialog: Update the developer list #2215
Merged

MapNotesDialog: Incorporate review proposals
521bb0e

dl3sdo marked this pull request as ready for review November 26, 2024 10:43

dl3sdoand others added 4 commits November 26, 2024 21:18

MapEditorController: Safeguard valid map pointer
b3ac125

MapEditorController: Remove now-unused headers
4fe3d47

MapNotesDialog: Use reference to map
d3fbb17

MapNotesDialog: Tune scaling
382ec0f
Consider app window size instead of screen size. Consider max line length for better readability.

dg0yt force-pushed the increase-map-notes branch from 73b32dd to 382ec0fCompare November 28, 2024 21:47

Merge remote-tracking branch 'origin/master' into increase-map-notes
a16bfb2

dg0yt merged commit 39ca931 into OpenOrienteering:masterDec 1, 2024

dl3sdo mentioned this pull request Jan 14, 2026
The notes window is very small #1117
Open

	setWindowTitle(QCoreApplication::translate("OpenOrienteering::MapNotes", "Map notes"));
	setWindowTitle(QCoreApplication::translate("OpenOrienteering::MapEditorController", "Map notes"));

	setWindowTitle(QCoreApplication::translate("OpenOrienteering::MapNotes", "Map notes"));
	setWindowTitle(tr("Map notes"));

		width = qMax(300, qMin(width, bounding_rect.width() + 60));
		height = qMax(200, qMin(height, bounding_rect.height() + 80));

Uh oh!

Conversation

dl3sdo commented Aug 5, 2024

Uh oh!

dl3sdo commented Aug 5, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dg0yt commented Nov 25, 2024

Uh oh!

dl3sdo commented Nov 25, 2024

Uh oh!

dg0yt left a comment

Choose a reason for hiding this comment

Uh oh!

dg0ytNov 26, 2024

Choose a reason for hiding this comment

Uh oh!

dl3sdoNov 26, 2024

Choose a reason for hiding this comment

Uh oh!

dl3sdoNov 26, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dg0ytNov 26, 2024

Choose a reason for hiding this comment

Uh oh!

dl3sdoNov 26, 2024

Choose a reason for hiding this comment

Uh oh!

dg0ytNov 26, 2024

Choose a reason for hiding this comment

Uh oh!

PappyEric commented Nov 26, 2024 via email

Uh oh!

dg0yt commented Nov 26, 2024

Uh oh!

PappyEric commented Nov 26, 2024 via email

Uh oh!

PappyEric commented Nov 26, 2024 via email

Uh oh!

dg0yt commented Nov 26, 2024

Uh oh!

PappyEric commented Nov 26, 2024 via email

Uh oh!

dg0yt commented Nov 26, 2024

Uh oh!

lpechacek commented Nov 26, 2024

Uh oh!

PappyEric commented Nov 26, 2024

Uh oh!

dl3sdo commented Nov 26, 2024

Uh oh!

dg0yt commented Nov 26, 2024

Uh oh!

dg0yt commented Nov 29, 2024

Uh oh!

dg0yt commented Nov 29, 2024

Uh oh!

azure-pipelinesBot commented Nov 29, 2024

Uh oh!

dg0yt commented Nov 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dl3sdo commented Aug 5, 2024•
edited
Loading

dl3sdoNov 26, 2024•
edited
Loading