Comment 1 Jonah Graham 2020-04-11 10:58:20 EDT

There are two bugs discussing this. Please see my comment in Bug 560614 Comment 11.

Comment 2 Eclipse Genie 2021-11-11 13:05:15 EST

New Gerrit change created: https://git.eclipse.org/r/c/cdt/org.eclipse.cdt/+/187651

Comment 3 Mat Booth 2021-11-11 13:13:14 EST

Created attachment 287493 [details]
String test script



After reading bug 560614 again, I decided to write a quick script (see attached file "find-interesting-messages.sh") to scrape out all the message strings from the cdt repo and test the "interesting" ones against both Java and ICU versions of MessageFormat. I ran it on current CDT master both before and after my change for this bug which finally removes the last of ICU4j https://git.eclipse.org/r/c/cdt/org.eclipse.cdt/+/187651


Before my change:
=====
$ ./find-interesting-messages.sh 
Already up to date.

4 ThreadVMNode_No_columns__text_format failed to parse with Java:
  Choice Pattern incorrect: 0#|1# #{3}
  Failing pattern: Thread{2,choice,0#|1# #{3}}{0,choice,0#|1# [{1}]}{4,choice,0#|1# {5}}{6,choice,0#|1# [core: {7}]} ({8,choice,0#Running|1#Suspended}{9,choice,0#|1# : {10}}{11,choice,0#|1# : {12}})

7 ThreadVMNode_No_columns__text_format_1636644442378412880 failed to parse with Java:
  Choice Pattern incorrect: 0#|1# #{3}
  Failing pattern: DSP{2,choice,0#|1# #{3}}{0,choice,0#|1# [{1}]}{4,choice,0#|1# {5}}{6,choice,0#|1# [core: {7}]} ({8,choice,0#Running|1#Suspended}{9,choice,0#|1# : {10}}{11,choice,0#|1# : {12}})
=====

It turns out there was two occurances of the problem discovered in bug 560614 where the pattern does not parse with java.text.MessageFormat.


After my change:
=====
$ ./find-interesting-messages.sh 
Already up to date.

4 ThreadVMNode_No_columns__text_format failed to parse with ICU:
  Unmatched '{' braces in message "Thread{2,choice,0#|1 ..."
  Failing pattern: Thread{2,choice,0#|1# '#'{3}}{0,choice,0#|1# [{1}]}{4,choice,0#|1# {5}}{6,choice,0#|1# [core: {7}]} ({8,choice,0#Running|1#Suspended}{9,choice,0#|1# : {10}}{11,choice,0#|1# : {12}})

7 ThreadVMNode_No_columns__text_format_1636651291101140911 failed to parse with ICU:
  Unmatched '{' braces in message "DSP{2,choice,0#|1# ' ..."
  Failing pattern: DSP{2,choice,0#|1# '#'{3}}{0,choice,0#|1# [{1}]}{4,choice,0#|1# {5}}{6,choice,0#|1# [core: {7}]} ({8,choice,0#Running|1#Suspended}{9,choice,0#|1# : {10}}{11,choice,0#|1# : {12}})
=====

This shows the same two strings this time not parsing with com.ibm.icu.text.MessageFormat.

Since we no longer require ICU4j anywhere in the CDT code base I don't think it's a problem that these specific strings now only parse with with java.lang.MessageFormat but we should definitely add a note to the N&N document about API change in o.e.cdt.dsf.ui.viewmodel.properties.LabelText

WDYT?

Comment 4 Jonah Graham 2021-11-11 13:42:21 EST

(In reply to Mat Booth from comment #3)
> WDYT?

Lets have the conversation here in the bug (I wrote some stuff in gerrit before I saw this)

Comment 5 Jonah Graham 2021-11-11 13:43:47 EST

The LabelText patch changes the signature of some methods - but LabelText is technically "provisional API" because the dependent packages in Platform Debug are provisional (see Bug 365672 and search [1] for "provisional"). 

This change would cause a runtime exception for extenders who are not building against the latest version. 

Therefore, before merging this I think it needs at a minimum some notice on cdt-dev, and possibly the two year deprecate & delete process[2].

WDYT?

[1] https://help.eclipse.org/latest/topic/org.eclipse.cdt.doc.isv/guide/dsf/intro/dsf_programming_intro.html?cp=14_0_6
[2] https://wiki.eclipse.org/CDT/policy#Deprecating_and_Deleting_API

Comment 6 Jonah Graham 2021-11-11 13:52:03 EST

One particular challenge is that the LabelText change is that at the source code level an extenders source can be compatible with no changes, but at the binary level it is incompatible. See org.eclipse.cdt.dsf.debug.ui.viewmodel.register.RegisterBitFieldVMNode.createLabelProvider().new FormattedValueLabelText() {...}.updateAttribute(ILabelUpdate, int, IStatus, Map<String, Object>) for an example in CDT of where that break would happen.

i.e. doing something like:

LabelText lt = ...
lt.getMessageFormat().format(...) 

would be source compatible with or without the gerrit.

----

In fact, if we are going to move the API forward I think that deleting the get/set MessageFormat + constructor is a better option as it makes it obvious what has changed. In that case setMessageFormat(MessageFormat messageFormat) can be replaced with 
setMessageFormatPattern(String formatPattern) {
    fMessageFormat = new MessageFormat(formatPattern);
}