Bug 361916

Comment 1 Deepak Azad 2011-10-25 08:58:57 EDT

> Remove Key(s) -> addl info is not very clear that it is modifying 2 different
> files. 
I was hoping that it would be clear :-)

> We use '...'  to show modifications in different parts of the same file
> for other proposals. I'm not sure if we also use it to show changes in 2
> different files already, but I think we probably should indicate it,  
I think this is the first time that the additional info shows changes from 2 files, at least I did not find any precedent.

> ex: mention the file names, in that case.
Yeah, we could show the file names (in bold?)

Comment 2 Dani Megert 2011-10-25 09:09:34 EDT

> > ex: mention the file names, in that case.
> Yeah, we could show the file names (in bold?)

The question is how valuable it is at all to show the changes for both files. Maybe it's enough to just say which files we change?

Comment 3 Dani Megert 2011-10-27 02:54:22 EDT

*** Bug 362102 has been marked as a duplicate of this bug. ***

Comment 4 Deepak Azad 2011-11-04 08:14:14 EDT

From Bug 362102

"
Improve the preview for 'Remove property' and 'Remove properties' proposals.

The current preview doesn't show anything interesting. Check how it looks if
you render the removed properties with strikeout style. Or put an explicit
"<removed property>" there.

Furthermore, this is the first quick assist that makes changes in multiple
files. The preview has to show where the changes will be performed (e.g. with
"Filename.ext:" in front of the section).
"

Comment 5 Deepak Azad 2011-11-08 12:21:12 EST

Created attachment 206614 [details]
screenshot with strikeout

I think strikeout looks good. Markus ?

Comment 6 Markus Keller 2011-11-08 12:54:05 EST

Screenshot looks good. Please add the file names, and then I guess we're done.

Comment 7 Dani Megert 2011-11-09 02:48:29 EST

(In reply to comment #6)
> Screenshot looks good. Please add the file names, and then I guess we're done.

+1.

Comment 8 Deepak Azad 2011-11-10 01:17:45 EST

Created attachment 206757 [details]
patch for jdt ui

Markus, as discussed 
- the patch uses strikeout style using 'del' tags
- EditAnnotator is not touched, this change is limited to Remove properties quick assist.
- File names are also displayed in the preview. (Can you please review the code for this part.) 

(In reply to comment #0)
> 1)use the standard 'Rename in workspace' shorcut for the proposal in the
> properties file
Done

Comment 9 Deepak Azad 2011-11-10 01:19:00 EST

Created attachment 206758 [details]
patch for platform text

Adds support for 'del' tag to HTML2TextReader.

Comment 10 Markus Keller 2011-11-10 09:43:24 EST

Comment on attachment 206758 [details]
patch for platform text

You have to make sure that the HTML2TextReader also works if the bold and strike-through ranges overlap. The patch will throw IAEs in that case.

To reproduce the problem, hack BrowserInformationControl#isAvailable(Composite) to always return false. Then show this Javadoc hover:

/**
 * Hello <del>deleted</del> world.<br>
 * Hello <b>bold</b> world.<br>
 * <b>Hello <del>deleted</del></b> world.<br>
 */
class TestClass { }


I guess the implementation also has to add a style range in the start*() method if the other style was already started before. And stop*() always has to set all enabled styles.

Comment 11 Markus Keller 2011-11-10 09:58:34 EST

The JDT UI patch looks good, except that it misses to align the additional info text of the Rename quick assist with the one from the Java editor:

(In reply to comment #0)
> Also, the text in addl info box shows 'Start the Rename refactoring' not 'Start
> Rename refactoring'

Comment 12 Deepak Azad 2011-11-13 10:25:08 EST

Created attachment 206901 [details]
patch for platform text

(In reply to comment #10)
> You have to make sure that the HTML2TextReader also works if the bold and
> strike-through ranges overlap. The patch will throw IAEs in that case.
Done. I also added tests to HTML2TextReaderTester to make sure that the StyleRanges do not overlap. 

The tests do not actually apply the StyleRanges to a StyledText widget, but only check for an overlap, which I thought was simpler to do :)

Comment 13 Deepak Azad 2011-11-13 10:27:42 EST

(In reply to comment #11)
> The JDT UI patch looks good, except that it misses to align the additional info
> text of the Rename quick assist with the one from the Java editor:

I have the change in my local repo. Will push it along with other changes when the platform text patch is good to go.

Comment 14 Markus Keller 2011-11-14 10:20:12 EST

(In reply to comment #12)
> Created attachment 206901 [details] [diff]
> patch for platform text

Thanks, released as commit 9652ad7296f5ebc4cb54564e1a47def3a8f879c3.

Comment 15 Deepak Azad 2011-11-14 10:46:04 EST

Thanks Markus!

Pushed JDT UI change as well - c76aef0f470c9e1d28c5b46d2c807072a8e10192

Comment 16 Raksha Vasisht 2011-12-06 06:50:11 EST

Verified for Juno M4 with 4.2 I20111205-1810.