Bug report #6647

Combobox text from custom form is not written to feature

Added by zicke - about 12 years ago. Updated over 11 years ago.

Status:Closed
Priority:Normal
Assignee:Matthias Kuhn
Category:GUI
Affected QGIS version:master Regression?:No
Operating System:Ubuntu Easy fix?:No
Pull Request or Patch supplied:Yes Resolution:
Crashes QGIS or corrupts data:No Copied to github as #:15827

Description

When using custom forms with comboboxes, the current text from the combobox is not written/saved to the feature.
This worked with QGIS 1.8.

qgsattributeeditor.diff Magnifier (1015 Bytes) zicke -, 2012-11-07 05:21 AM

atrr-editor_patch.diff Magnifier (3.56 KB) Larry Shaffer, 2012-12-16 09:40 PM

attr-editor_combobox-filll_patch.diff Magnifier (865 Bytes) Larry Shaffer, 2012-12-17 11:08 PM

History

#1 Updated by Giovanni Manghi about 12 years ago

  • Priority changed from Normal to Severe/Regression
  • Status changed from Open to Feedback

then is a regression in qgis master? if yes then is a blocker.

#2 Updated by Matthias Kuhn about 12 years ago

Can you provide the steps to reproduce this?

An autogenerated form with a Value map works here.

#3 Updated by Matthias Kuhn about 12 years ago

  • Assignee set to Matthias Kuhn

#4 Updated by zicke - about 12 years ago

I think it's the new QgsAttributeEditor::createAttributeEditor() method. If you got a combobox in your custom feature form and the edit type is line edit (which I think is still reasonable) the attribute editor is not created and the widget is not inserted into proxyWidgets. Then in QgsAttributeEditor::retrieveValues() the "null" widget is not casted to QComboBox.

Solution: see attached diff file (not sure if anything other is affected).

#5 Updated by Giovanni Manghi about 12 years ago

  • Pull Request or Patch supplied changed from No to Yes

#6 Updated by Matthias Kuhn about 12 years ago

Pull request awaiting approval
https://github.com/qgis/Quantum-GIS/pull/320

#7 Updated by Matthias Kuhn about 12 years ago

  • Target version set to Version 2.0.0
  • Status changed from Feedback to Resolved

#8 Updated by Matthias Kuhn about 12 years ago

  • Status changed from Resolved to Closed

#9 Updated by Larry Shaffer almost 12 years ago

  • Category set to GUI
  • Status changed from Closed to Reopened

Hi Matthias,

I am reopening this because the method used for the synchronization between the QComboBox and QLineEdit causes a cyclical signal/slot issue that exhibits as following (at least on Mac with Qt 4.8.3 and latest master build):

Any keyboard input (add/delete character, copy/paste, etc.) immediately causes the cursor to jump to end of the text block, making editing very frustrating to near impossible for larger edits.

I have a pull request in that works for me, but would like you to look over it, edit as you see fit, or do something completely different:

https://github.com/qgis/Quantum-GIS/pull/361

#10 Updated by Larry Shaffer almost 12 years ago

Here's a patch for that pull request

#11 Updated by Matthias Kuhn almost 12 years ago

Hi Larry,

I'm busy at the moment, so I don't have the time to look at it in depth. Am I correct, that your patch does only emit the signal, once the editing is finished?
If yes, that sounds reasonable and if it solves this problem it can be merged.

The optimal method would be to compare the current text to the provided text in the setText slot of the edit widgets, and in case they're the same do not perform any update. I thought that was already implemented, but obviously the update of the widget happens and only the reemitting of the signal is inhibited.
That solution would probably require a reasonable amount of extra work (with the only benefit of a nicer GUI feedback), so I'm happy to go with your solution.

#12 Updated by Matthias Kuhn almost 12 years ago

Another idea, pretty simple and straightforward:

In QgsStringRelay check which widget emitted the signal and skip this widget while re-emitting.

Would that work?

#13 Updated by Larry Shaffer almost 12 years ago

Hmm,

Matthias, could you explain where the text synchronization between QComboBox and QLineEdit is used, or visible?

I commented out the QgsStringRelay and connections in qgsattributeeditor.cpp (currently line 505-533) and there is no functional difference when the base field type is QLineEdit and the custom ui form is a QComboBox. Using the attached diff for loading the field's data in the editable QComboBox made that custom widget work like any other, with its text value being loaded and it edits saved back to the attribute table.

I left open the Attribute Table editor while using the custom attribute form for a feature and there was no synchronization between the custom form's QComboBox and the editor's QLineEdit (using original code, before editing or commenting out).

After looking at the connections again, they appear to build a cyclical signal/slot to the same widget, via the relay. So, my pull request changes (with regards to synchronization) appear to only fix the cursor-jumping issue while editing.

I think the synchronization is broken, but not sure where to see/test it, since the custom form's QComboBox is working fine.

Thanks.

#14 Updated by Matthias Kuhn almost 12 years ago

The synchronisation works between several widgets for the same field on the same custom form. In case you want a particular field to be shown on different parts of the form (e.g. different tabs). You can create a test-case by creating a custom form with the drag and drop editor in the vector layer properties and dragging the same field twice onto the form. Another option is to give two widgets in a UI-file the same name (you have to use an external editor for this, qt designer does not support this)

Synchronisation has only been implemented for line edits so far, and was intended as a reference implementation. For other types, a second widget is shown read-only to prevent ambiguity when saving.

If there appear too many problems with this implementation, we can disable synchronisation for line edits as well (although I like it)

I think breaking the cyclic nature of the concept by only relaying to widgets which not emitted the original signal should solve this problem.

#15 Updated by Larry Shaffer almost 12 years ago

Matthias Kuhn wrote:

The synchronisation works between several widgets for the same field on the same custom form. In case you want a particular field to be shown on different parts of the form (e.g. different tabs). You can create a test-case by creating a custom form with the drag and drop editor in the vector layer properties and dragging the same field twice onto the form. Another option is to give two widgets in a UI-file the same name (you have to use an external editor for this, qt designer does not support this)

Synchronisation has only been implemented for line edits so far, and was intended as a reference implementation. For other types, a second widget is shown read-only to prevent ambiguity when saving.

If there appear too many problems with this implementation, we can disable synchronisation for line edits as well (although I like it)

I think breaking the cyclic nature of the concept by only relaying to widgets which not emitted the original signal should solve this problem.

Thanks for the very clear explanation, though I feel a little silly for not noticing how the syncing was used. :^)

I have a new pull request in that keeps the text syncing and fixes the cyclical updating issue. Tested with custom form with two QLineEdits and an editable QComboBox for same attribute, all with full live synchronization between them. Also tested with same QLineEdit in two different tabs via the drag/drop interface. I believe this new fix uses the basic idea you were suggesting and is a flexible approach that can be used for syncing other widgets (please review):

https://github.com/qgis/Quantum-GIS/pull/363

#16 Updated by Larry Shaffer almost 12 years ago

  • Priority changed from Severe/Regression to High

Matthias Kuhn wrote:

Now it only works for QLineEdit/QComboBox I guess? I think QPlainTextEdit and QTextEdit should be supported as well.
Maybe it's possible to keep it generic? Not sure if that's easy, that would probably mean keeping a local list of connected slots which can be scanned for slots of QObject::sender() to disconnect.

I have made it as generic as I can (slot's signature is passed in QObject property). Works well now, syncing across all of the text editor widgets... excepting for validation. :^(

https://github.com/dakcarto/Quantum-GIS/commit/cdfd8da9a4f53331cca5138d22c3658f0f0471db

I added a QgsFieldValidator to editable QComboBoxes (was missing), but there is no easy way to add field-relative validation to Q[Plain]TextEdit widgets (at least not yet). For example, when a user edits in a QLineEdit or QComboBox, the validation is done (e.g. max number of characters). But, when the user switches to a synchronized QPlainText widget, they can effectively override the validation.

Unfortunately, this is not evident unless the user saves the edits, which would write a NULL to the table, or fail to write. If, during editing, the user switches back to the QLineEdit or QComboBox, which now contains invalid text, still no validation happens. Only when leaving the QLineEdit or QComboBox does visual feedback for validation kick in, thereby turning all synced widgets to NULL. I think there needs to be better feedback for validation.

So, while there is nothing wrong with my latest syncing fix (as far as I can tell), there is currently no field validation for Q[Plain]TextEdit widgets, unless I'm missing it.

#17 Updated by Matthias Kuhn almost 12 years ago

Thank you for your work. I did not try it, but that pull request looks good to me.

The flaws concerning validation are no regression, right? Or is there a situation, that was validated before and is now not any more? Is this only happening, when using different widgets for the same attribute? If yes, that would be a nice improvment, no doubt, but no blocker to merge this pull request.

I would merge this pull request and close this bug as soon as it's merged.
We can then open a new feature request for validation of Q[Plain]TextEdit.

#18 Updated by Giovanni Manghi almost 12 years ago

  • Priority changed from High to Normal

#19 Updated by Matthias Kuhn over 11 years ago

Larry, what's the status of this? Did you merge your changes?
(I guess you did, I heard nobody complaining about uncommitted changes lately.)

If yes, we should close this ticket and open a new one, asking for field validation for Q[Plain]TextEdit widgets.

#20 Updated by Larry Shaffer over 11 years ago

Yes, I committed my updates to it a while back. Don't know about this particular issue, though. The work I did was to keep the cyclical updates from happening. Never tested the actual issue here.

#21 Updated by Matthias Kuhn over 11 years ago

In this case I'll close it.
My changes should have fixed the original issue and yours the remainder.

#22 Updated by Matthias Kuhn over 11 years ago

  • Status changed from Reopened to Closed

Also available in: Atom PDF