Modify

Opened 3 years ago

Closed 3 years ago

Last modified 20 months ago

#21319 closed enhancement (fixed)

[PATCH] Refactoring of class hierarchy around JosmComboBox / AutoCompComboBox

Reported by: marcello@… Owned by: team
Priority: major Milestone: 21.10
Component: Core Version:
Keywords: Cc:

Description

Added classes in bold to orthogonalize hierarchy:

JComboBox JComboBoxModel JComboBoxEditor JTextField
JosmComboBox JosmComboBoxModel JosmComboBoxEditor JosmTextField
AutoCompComboBox AutoCompComboBoxModel AutoCompComboBoxEditor AutoCompTextField

Code reuse: JosmComboBox now uses a JosmTextField as editor
Code reuse: AutoCompComboBox now uses AutoCompTextField as editor
JosmComboBox uses more of the original L&F
JosmComboBox lists now expand all the way to the bottom or the top of the screen
Variable height items in combobox lists now work #19321
Autocomplete uses different algorithm #21290
editable="false" comboboxes in Presets now work #6157 #11024 #18714
The user may toggle LTR-RTL script in JosmTextField (menu and ctrl+space)
LTR-RTL automatically toggles according to key in AddTag and EditTag dialogs #16163

Attachments (3)

21319.patch (181.8 KB ) - added by marcello@… 3 years ago.
josm_preset_missing_optional_text.jpg (35.5 KB ) - added by skyper 3 years ago.
missing text
21319-2.patch (867 bytes ) - added by marcello@… 3 years ago.

Download all attachments as: .zip

Change History (36)

by marcello@…, 3 years ago

Attachment: 21319.patch added

comment:1 by Don-vip, 3 years ago

Wow, that's quite a patch

comment:2 by Don-vip, 3 years ago

Does it mean your patch solves #21290, #6157 and #16163 all at once?

comment:3 by Don-vip, 3 years ago

Milestone: 21.09

comment:4 by marcello@…, 3 years ago

It fixes #6157, proposes a solution for #16163, and hopefully fixes #21290 (lacking a Mac I cannot test it).

comment:5 by Don-vip, 3 years ago

excellent!

comment:6 by Don-vip, 3 years ago

Priority: normalmajor

comment:7 by Don-vip, 3 years ago

Resolution: fixed
Status: newclosed

In 18221/josm:

fix #21319 - Refactoring of class hierarchy around JosmComboBox / AutoCompComboBox (patch by marcello):

  • Code reuse: JosmComboBox now uses a JosmTextField as editor
  • Code reuse: AutoCompComboBox now uses AutoCompTextField as editor
  • JosmComboBox uses more of the original L&F
  • JosmComboBox lists now expand all the way to the bottom or the top of the screen
  • Variable height items in combobox lists now work, see #19321
  • Autocomplete uses different algorithm, fix #21290
  • editable="false" comboboxes in Presets now work, fix #6157 see #11024 see #18714
  • The user may toggle LTR-RTL script in JosmTextField (menu and ctrl+space)
  • LTR-RTL automatically toggles according to key in AddTag and EditTag dialogs, fix #16163

comment:8 by Don-vip, 3 years ago

In 18222/josm:

see #21319 - fix NPE

by skyper, 3 years ago

missing text

comment:9 by skyper, 3 years ago

I have some missing <optional> with or without additional text which only appear when I open the preset for the second time. The first time opening the preset after restart/reload empty space is present but the text is missing. Is this related to this ticket?

missing working
missing textwater_well preset screenshot

comment:10 by skyper, 3 years ago

Resolution: fixed
Status: closedreopened

Just tried with r18218 which works as expected, so I blame r18221.

by marcello@…, 3 years ago

Attachment: 21319-2.patch added

comment:11 by marcello@…, 3 years ago

Oops. Sorry. Patch.

comment:12 by Don-vip, 3 years ago

Resolution: fixed
Status: reopenedclosed

In 18223/josm:

fix #21319 - fix optional regression (patch by marcello)

comment:13 by skyper, 3 years ago

Does this patch also fix `<optional text="Hallo world">?

Version 0, edited 3 years ago by skyper (next)

comment:14 by marcello@…, 3 years ago

where do you see that?

comment:15 by skyper, 3 years ago

<optional> has the option to add text instead of the default with text="".

In my screenshots above it is <optional text="Only in case of manual actuator or manual mechanical drive: "> and one of many examples in current defaultpresets.xml is <optional text="Edit Highway Attributes:"> in "Cycle Lane/Track" preset.

comment:16 by skyper, 3 years ago

Just tried r18224 and it works. Thanks for the quick fix.

comment:17 by gaben, 3 years ago

Hmm, combo boxes don't seem to work when a value has already entered. You can check it eg. in the restaurant preset.

comment:18 by gaben, 3 years ago

Ah sorry, multiselect is broken, not combobox. r18218 works, r18222 not.

in reply to:  18 comment:19 by skyper, 3 years ago

Replying to gaben:

Ah sorry, multiselect is broken, not combobox. r18218 works, r18222 not.

Cannot reproduce with r18237 and flatlaf (Dracula).

comment:20 by gaben, 3 years ago

For me, both Win and Linux affected with default LaF, r18237. @skyper, can you please test with native laf?

comment:21 by gaben, 3 years ago

Oh okay, so the multiselect is only "broken" when the tag has only a single value. Also if the value contains extra space between the semicolon separators eg. cuisine=asian; african.

By broken I mean greyed out, so I can not use the GUI for adding items.

comment:22 by skyper, 3 years ago

Ok, I can reproduce it if an unknown value is present (including leading or trailing white spaces). This is nothing new and happens with r18193, too. The problem is that, atm, unlike combo no values are add to the list and cause of that not all values are displayed. To stop users from overwriting valid but unknown values the multiselect is disabled. Please, open a new enhancement ticket, if you think this should be changed. Thanks.

comment:23 by gaben, 3 years ago

Created a new ticket, see #21385.

comment:24 by skyper, 3 years ago

Another possible regression: #21397

comment:25 by Don-vip, 3 years ago

Milestone: 21.0921.10

Milestone renamed

comment:26 by GerdP, 3 years ago

Another possible regression: #21438

comment:27 by GerdP, 2 years ago

another regression? #21619

comment:28 by taylor.smock, 2 years ago

Another regression: #22055

comment:29 by taylor.smock, 21 months ago

In 18538/josm:

JosmTextField: setHint now properly returns the old hint

See #21319/r18221. This additionally adds a non-regression test.

comment:30 by Klumbumbus, 20 months ago

There are 62 unit tests failing on Jenkins since r18538. Not sure if this is a problem on Jenkins though, since there are also 2 test failing after r18537, which doens't make sense imo.

in reply to:  30 comment:31 by taylor.smock, 20 months ago

Replying to Klumbumbus:

There are 62 unit tests failing on Jenkins since r18538. Not sure if this is a problem on Jenkins though, since there are also 2 test failing after r18537, which doens't make sense imo.

The OSM dev server was down (errol). So all the integration tests expecting a live dev api failed.

comment:32 by Klumbumbus, 20 months ago

Ah, OK. I just started a new run on Jenkins. Lets see if the tests pass this time. --> they did.

Last edited 20 months ago by Klumbumbus (previous) (diff)

in reply to:  32 comment:33 by taylor.smock, 20 months ago

Replying to Klumbumbus:

Ah, OK. I just started a new run on Jenkins. Lets see if the tests pass this time. --> they did.

That is kind of what I expected. I don't know how prevalent the dev server going down is going to be (this is the second or third time this year). There was an outage in July as well. I tracked that one in #22216.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.