Modify ↓
#21240 closed enhancement (fixed)
[PATCH] [RFC] Refactoring of UploadDialog, HistoryComboBox and AutoCompletingComboBox
| Reported by: | Owned by: | Don-vip | |
|---|---|---|---|
| Priority: | major | Milestone: | 21.08 |
| Component: | Core | Version: | |
| Keywords: | Cc: |
Description
This is quite a patch so it should be reviewed before applying.
I started trying to fix the long-standing upload comment off-by-one error. I found that UploadDialog was more complex that it needed to be and too many things could go wrong, see the many unsuccessful attempts to fix the off-by-one error. So I rewrote and simplified it. In the course I found that HistoryComboBox was buggy and could contribute to the error, and its class hierarchy with AutoCompletingComboBox was in need of refactoring.
Salient points:
- Refactoring of
AutoCompletingComboBox- new
AutoCompComboBox<E>is a genericAutoCompletingComboBox- you can put any object into it and get it out again unchanged
- the combobox will display the object's
toString() - the combobox will autocomplete on all the objects'
toString() - autocomplete can use
Stringcompare or custom comparators to find the best item
HistoryComboBoxnow extendsAutoCompComboBox<String>- no more
AutoCompletionItemsused inHistoryComboBox - interface has been cleaned up (functions deprecated)
- no more
- deprecated
AutoCompletingComboBoxnow is a stub that extendsAutoCompComboBox<AutoCompletionItem> - new
AutoCompComboBoxModel<E>andHistoryComboBoxModelhave been provided- non-UI functions have been moved to the models
- autocomplete now implemented as key listener
- not overriding internal document functions (no more risk of loops)
- added tests
- new
- Refactoring of
UploadDialog- Fixes the long-standing upload comment off-by-one bug
- Four models (2 combobox, 1 checkbox, 1 tageditor) have been consolidated into one model
- Made sure all controls update their models before uploading (eg. Ctrl-Enter shortcut)
- Added tests
Probably more rewriting of UploadDialog is needed to make it clean.
Comments are welcome.
Attachments (4)
Change History (21)
by , 3 years ago
| Attachment: | 21240.patch added |
|---|
comment:1 by , 3 years ago
| Milestone: | → 21.08 |
|---|---|
| Owner: | changed from to |
| Priority: | normal → major |
| Status: | new → assigned |
comment:2 by , 3 years ago
I'm testing it and faced this when switching from Description tab to Settings tab:
Revision:18157 Is-Local-Build:true Build-Date:2021-08-22 16:10:54 Identification: JOSM/1.5 (18157 SVN en) Windows 10 64-Bit OS Build number: Windows 10 Pro 2009 (19042) Memory Usage: 268 MB / 4088 MB (114 MB allocated, but free) Java version: 16.0.1+9, AdoptOpenJDK, OpenJDK 64-Bit Server VM Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel Screen: \Display0 2560×1440 (scaling 1.00×1.00) \Display1 1920×1080 (scaling 1.00×1.00) Maximum Screen Size: 2560×1440 Best cursor sizes: 16×16→32×32, 32×32→32×32 System property file.encoding: UTF-8 System property sun.jnu.encoding: Cp1252 Locale info: en_FR Numbers with default locale: 1234567890 -> 1234567890 VM arguments: [-ea, --add-opens=java.base/java.io=ALL-UNNAMED, --add-opens=java.base/java.lang=ALL-UNNAMED, --add-opens=java.base/java.nio=ALL-UNNAMED, --add-opens=java.base/java.text=ALL-UNNAMED, --add-opens=java.base/java.util=ALL-UNNAMED, --add-opens=java.base/jdk.internal.loader=ALL-UNNAMED, --add-opens=java.desktop/java.awt=ALL-UNNAMED, --add-opens=java.desktop/javax.swing.text.html=ALL-UNNAMED, --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED, -Dfile.encoding=UTF-8] Dataset consistency test: No problems found Plugins: + apache-commons (35524) + ejml (35458) + geotools (35458) + jaxb (35543) + jts (35458) + opendata (35803) + utilsplugin2 (35792) Last errors/warnings: - 00000.519 W: extended font config - overriding 'filename.Myanmar_Text=mmrtext.ttf' with 'MMRTEXT.TTF' - 00000.522 W: extended font config - overriding 'filename.Mongolian_Baiti=monbaiti.ttf' with 'MONBAITI.TTF' - 00116.778 E: Handled by bug report queue: java.lang.AssertionError === REPORTED CRASH DATA === BugReportExceptionHandler#handleException: No data collected. Warning issued by: BugReportExceptionHandler#handleException === STACK TRACE === Thread: AWT-EventQueue-0 (22) of main java.lang.AssertionError at org.openstreetmap.josm.gui.io.BasicUploadSettingsPanel.updateHistory(BasicUploadSettingsPanel.java:318) at org.openstreetmap.josm.gui.io.BasicUploadSettingsPanel.focusLost(BasicUploadSettingsPanel.java:380) at java.desktop/java.awt.AWTEventMulticaster.focusLost(AWTEventMulticaster.java:238) at java.desktop/java.awt.Component.processFocusEvent(Component.java:6509) at java.desktop/java.awt.Component.processEvent(Component.java:6373) at java.desktop/java.awt.Container.processEvent(Container.java:2264) at java.desktop/java.awt.Component.dispatchEventImpl(Component.java:4993) at java.desktop/java.awt.Container.dispatchEventImpl(Container.java:2322) at java.desktop/java.awt.Component.dispatchEvent(Component.java:4825) at java.desktop/java.awt.KeyboardFocusManager.redispatchEvent(KeyboardFocusManager.java:1950) at java.desktop/java.awt.DefaultKeyboardFocusManager.typeAheadAssertions(DefaultKeyboardFocusManager.java:1064) at java.desktop/java.awt.DefaultKeyboardFocusManager.dispatchEvent(DefaultKeyboardFocusManager.java:730) at java.desktop/java.awt.Component.dispatchEventImpl(Component.java:4874) at java.desktop/java.awt.Container.dispatchEventImpl(Container.java:2322) at java.desktop/java.awt.Component.dispatchEvent(Component.java:4825) at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:772) at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:721) at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:715) at java.base/java.security.AccessController.doPrivileged(AccessController.java:391) at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:85) at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:95) at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:745) at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:743) at java.base/java.security.AccessController.doPrivileged(AccessController.java:391) at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:85) at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:742) at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203) at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124) at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:117) at java.desktop/java.awt.WaitDispatchSupport$2.run(WaitDispatchSupport.java:190) at java.desktop/java.awt.WaitDispatchSupport$4.run(WaitDispatchSupport.java:235) at java.desktop/java.awt.WaitDispatchSupport$4.run(WaitDispatchSupport.java:233) at java.base/java.security.AccessController.doPrivileged(AccessController.java:312) at java.desktop/java.awt.WaitDispatchSupport.enter(WaitDispatchSupport.java:233) at java.desktop/java.awt.Dialog.show(Dialog.java:1079) at java.desktop/java.awt.Component.show(Component.java:1720) at java.desktop/java.awt.Component.setVisible(Component.java:1667) at java.desktop/java.awt.Window.setVisible(Window.java:1032) at java.desktop/java.awt.Dialog.setVisible(Dialog.java:1014) at org.openstreetmap.josm.gui.io.UploadDialog.setVisible(UploadDialog.java:361) at org.openstreetmap.josm.actions.UploadAction.uploadData(UploadAction.java:244) at org.openstreetmap.josm.actions.UploadAction.actionPerformed(UploadAction.java:292) at java.desktop/javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:1972) at java.desktop/javax.swing.AbstractButton$Handler.actionPerformed(AbstractButton.java:2313) at java.desktop/javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:405) at java.desktop/javax.swing.DefaultButtonModel.setPressed(DefaultButtonModel.java:262) at java.desktop/javax.swing.plaf.basic.BasicButtonListener.mouseReleased(BasicButtonListener.java:279) at java.desktop/java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:297) at java.desktop/java.awt.Component.processMouseEvent(Component.java:6617) at java.desktop/javax.swing.JComponent.processMouseEvent(JComponent.java:3342) at java.desktop/java.awt.Component.processEvent(Component.java:6382) at java.desktop/java.awt.Container.processEvent(Container.java:2264) at java.desktop/java.awt.Component.dispatchEventImpl(Component.java:4993) at java.desktop/java.awt.Container.dispatchEventImpl(Container.java:2322) at java.desktop/java.awt.Component.dispatchEvent(Component.java:4825) at java.desktop/java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4934) at java.desktop/java.awt.LightweightDispatcher.processMouseEvent(Container.java:4563) at java.desktop/java.awt.LightweightDispatcher.dispatchEvent(Container.java:4504) at java.desktop/java.awt.Container.dispatchEventImpl(Container.java:2308) at java.desktop/java.awt.Window.dispatchEventImpl(Window.java:2773) at java.desktop/java.awt.Component.dispatchEvent(Component.java:4825) at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:772) at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:721) at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:715) at java.base/java.security.AccessController.doPrivileged(AccessController.java:391) at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:85) at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:95) at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:745) at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:743) at java.base/java.security.AccessController.doPrivileged(AccessController.java:391) at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:85) at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:742) at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203) at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124) at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:113) at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:109) at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101) at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:90)
comment:3 by , 3 years ago
Indeed in debug, text.getParent().getParent() is a JPanel instance. text.getParent() is an HistoryComboBox instance.
comment:4 by , 3 years ago
I've changed text.getParent().getParent() to text.getParent(). The assertion is gone but the widgets do not work. I can't enter anything in comment text field: every time I type a character it erases the previous one. Can you please check with my patch (I've addressed a few code style issues)?
by , 3 years ago
| Attachment: | 21240_v1_modified.diff added |
|---|
comment:5 by , 3 years ago
| Owner: | changed from to |
|---|---|
| Status: | assigned → needinfo |
comment:6 by , 3 years ago
Maybe Swing builds a combobox in different ways on Linux and Windows. I have to find a more robust way to get to the combobox.
comment:7 by , 3 years ago
Uploaded revised patch tested on Windows. Windows selects the text on setItem() whereas Linux does not. Linux also uses more Components to build a ComboBox than Windows.
comment:8 by , 3 years ago
| Owner: | changed from to |
|---|---|
| Status: | needinfo → assigned |
OK it seems to work flawlessly now! I haven tested a lot of things in upload dialog, everything looks fine. Applying the patch right now :)
comment:12 by , 3 years ago
the HouseNumberTaggingTool plugin is broken since the refactoring, can you please take a look?
browser/osm/applications/editors/josm/plugins/HouseNumberTaggingTool
by , 3 years ago
| Attachment: | 21240.housenumber.patch added |
|---|
Note:
See TracTickets
for help on using tickets.
Wow, thanks! Taking a look