Commit
…f attributes is open) Dual view's attribute form was triggering canvas refresh which in turn caused the popup menu to be closed again. (at least in KDE)
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -237,7 +237,12 @@ bool QgsAttributeForm::save() | |
|
||
emit featureSaved( updatedFeature ); | ||
|
||
mLayer->triggerRepaint(); | ||
// [MD] disabled trigger of repaint as it interferes with other stuff (#11361). | ||
// This code should be revisited - and the signals should be fired (+ layer repainted) | ||
// only when actually doing any changes. I am unsure if it is actually a good idea | ||
// to call save() whenever some code asks for vector layer's modified status | ||
// (which is the case when attribute table is open) | ||
//mLayer->triggerRepaint(); | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong.
wonder-sk
Author
Member
|
||
|
||
mIsSaving = false; | ||
|
||
|
7 comments
on commit 5e54912
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would trigger a new item on the undo stack for every single keystroke in a text edit widget. I am concerned that this will decrease software usability considerably.
I think a two-step procedure would be good.
Sometimes you are only interested if there have been modifications and not which modifications there are (E.g. Color of the pencil in the layer tree), in such a case there is no need to call save()
. In such a situation you call isModified()
which could be implemented with less overhead. A second call savePendingChanges()
could then be used which actually triggers the save()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also QLineEdit::editingFinished() signal which would avoid modification of undo stack on every single keystroke. Moreover with merging of undo commands (supported by Qt undo framework) all changes to one feature/attribute could be merged just into the last one command. I have seen this implemented elsewhere (with Qt) at it is really fast. Even if such operation required a huge amount of time (e.g. 10 ms), users would need to type ~100 characters per second to see any difference :-)
The two-step procedure would of course help here too, I am just a bit worried that handling of edit buffer would get more complicated due to that. But I may be wrong :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you type in a QLineEdit, leave the cursor there and click the save layer button. Would that also trigger the editingFinished() signal?
My main concern was less the performance while typing than undo actually being rendered useless because you have to click it 100 times to undo every single keystroke. But merging of undo commands sounds interesting, that could be an approach indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the editingFinished() signal should be triggered because prior to the click the line edit should loose the focus, producing that signal before the actual click happens. But I am not too keen to use that signal if we can have merging of "change attribute value" undo commands...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was requested so that the canvas is refreshed when you add a new feature.
Now, when you have drawn the feature it is not shown on map canas.
This would be a big blocker.
Shall I fill a bug or do you have a solution in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm this regression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the
triggerRepaint()
was a leftover from some older code (or I must have been using the dizzy mode while writing this code).The point of saving in the modified check is that the layer actually is being modified without any extra buttons. It would probably be better to be able to notify about pending changes without actually writing them to the edit buffer yet and have another mechanism that lets forms (or other objects) write pending changes to the edit buffer when necessary (e.g. before saving the layer).