Skip to content
Permalink
Browse files
Fix #11361 (Cannot open menu for vector layer in TOC when its table o…
…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
wonder-sk committed Oct 20, 2014
1 parent 447ec0f commit 5e54912565930019bac14908658263ef92ee0426
Showing with 6 additions and 1 deletion.
  1. +6 −1 src/gui/qgsattributeform.cpp
@@ -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.

Copy link
@m-kuhn

m-kuhn Oct 20, 2014

Member

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).

This comment has been minimized.

Copy link
@wonder-sk

wonder-sk Oct 20, 2014

Author Member

@mkuhn you were not really dizzy - that code was added somewhat later by @3nids

What about adding undo commands at the time the form is being edited? With support for merging of undo commands this should have low overhead. One thing I am slightly concerned is that things can get a bit unpredictable with this kind of lazy save from isModified() call. For example, if it would be called from a worker thread - this would end up emitting a signals in a thread where we don't want to process them (or do anything GUI related)...


mIsSaving = false;

7 comments on commit 5e54912

@m-kuhn

This comment has been minimized.

Copy link
Member

@m-kuhn m-kuhn replied Oct 20, 2014

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().

@wonder-sk

This comment has been minimized.

Copy link
Member Author

@wonder-sk wonder-sk replied Oct 20, 2014

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 :-)

@m-kuhn

This comment has been minimized.

Copy link
Member

@m-kuhn m-kuhn replied Oct 20, 2014

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.

@wonder-sk

This comment has been minimized.

Copy link
Member Author

@wonder-sk wonder-sk replied Oct 20, 2014

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...

@3nids

This comment has been minimized.

Copy link
Member

@3nids 3nids replied Oct 21, 2014

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?

@gioman

This comment has been minimized.

Copy link
Contributor

@gioman gioman replied Oct 21, 2014

I can confirm this regression.

@gioman

This comment has been minimized.

Copy link
Contributor

@gioman gioman replied Oct 21, 2014

Please sign in to comment.