Major rework of Map's template API and of TemplateListWidget #1653
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lpechacek
changed the title
Move separate inline definitions either to class or to cpp file: We don't need to force inlining for getters, and we probaly don't want inlining for setters. Consistently use identifier "pos" for the index of the template (but leaving "i" for closed templates). Use idiomatic non-member begin(). Fix implicit conversions. Rewrite findTemplateIndex using std algorithms. Revise documentation.
Change addTemplate and setTemplate parameter order to the more common position, template. This also matches the signals.
Add method to move templates in the list, and send a new signal.
Explicitly take, hold, and release strong ownership of templates by using std::unique_ptr<Template>.
Update the index of the first front template when templates are added or removed in the background of the map. Introduce the special value '-1' to insert templates just below the map. Rewrite ReopenTemplateDialog::OpenTemplateList::dropEvent to make use of this API change. Note that the changed interactions at template deletion require another TemplateListWidget connection to be marked as queued: When the template table selection model emits selectionChanged, the template table still holds the row for the template which is already removed from Map, so table row <-> template index mappings do not work correctly in this moment.
Emit signals before adding, moving or removing templates, and when changing the index of the first front template. This follows the pattern of signals in QAbstractItemModel. It allows to do some light-weight independent actions, e.g. initializing template visibility, before more complex actions.
... when changing the list of templates, changing the start of the front templates, or being notified of a changed template. Note: This is the combined state of the templates with regard to what is saved on disk. It is not the state for repainting the screen.
For all common events, Map knows that setTemplateAreaDirty needs to be called (and when). In addition, a change of template visibility shouldn't affect more than the map widgets attached to the particular map view.
This change leverages Qt's model/view classes, separating the template list GUI from the the data model. To let the model properly deal with templates being added to the map next to the map layer, the map must also explicitly signal whether a template is meant to be added to the background or not. To avoid a hard dependency on map.h in target slot declarations, we use a simple bool, not a more verbose enum in map.h.
Change order of functions for reasonable grouping. Add '*' to 'auto' where dealing with pointers. Rename 'getCurrentTemplate' to 'currentTemplate'.
Tear down the tool when the user select another row in the list. Fixes GH-1517 (unintuitive move-template function).
Move full lifecycle control to TemplateListWidget and TemplatePositionDockWidget.
Turn these dock widget into regular MapEditorController/QMainWindow dock widgets for which state is saved and restored. Dock widgets need an object name in order to let QMainWindow properly save and restore the state. Fixes a runtime warning at the console.
lpechacek
reviewed
Jun 12, 2020
| @@ -1469,7 +1471,7 @@ protected slots: | |||
| private: | |||
| typedef std::vector<MapColor*> ColorVector; | |||
| typedef std::vector<Symbol*> SymbolVector; | |||
| typedef std::vector<Template*> TemplateVector; | |||
| typedef std::vector<std::unique_ptr<Template>> TemplateVector; | |||
There was a problem hiding this comment.
The use of unique_ptr obsoletes delete placeholder; in Importer::validate() (file_import_export.cpp). In fact, the delete leads to a crash.
There was a problem hiding this comment.
Okay. Will look into this tonight.
There was a problem hiding this comment.
Release notes updated.
Side note: I meant to test the binaries once more before publishing the draft release, but accidenty hit the wrong button.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move responsibilites from client code to
Mapwhere reasonable.Factor our a proper
TemplateTableModelfromTemplateListWidget.