Navigation Menu

Skip to content

Commit

Permalink
Fix #11361 (Cannot open menu for vector layer in TOC when its table o…
Browse files Browse the repository at this point in the history
…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 5e54912
Showing 1 changed file with 6 additions and 1 deletion.
7 changes: 6 additions & 1 deletion src/gui/qgsattributeform.cpp
Expand Up @@ -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;

Expand Down

7 comments on commit 5e54912

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 5e54912 Oct 20, 2014

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

@wonder-sk
Copy link
Member Author

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

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 5e54912 Oct 20, 2014

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.

@wonder-sk
Copy link
Member Author

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

@3nids
Copy link
Member

@3nids 3nids commented on 5e54912 Oct 21, 2014

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?

@gioman
Copy link
Contributor

@gioman gioman commented on 5e54912 Oct 21, 2014

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.

@gioman
Copy link
Contributor

@gioman gioman commented on 5e54912 Oct 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.