Bugzilla – Full Text Bug Listing
| Summary: | [api][rulers][preferences] Add preference that allows to disable Overview ruler color wash out | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Ron Ratovsky <webron> | ||||||||||||||||||
| Component: | Text | Assignee: | Raksha Vasisht <raksha.vasisht> | ||||||||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||||
| Priority: | P2 | CC: | daniel_megert, markus.kell.r, pascal, remy.suen | ||||||||||||||||||
| Version: | 3.6.2 | Keywords: | accessibility, api | ||||||||||||||||||
| Target Milestone: | 3.8 M3 | Flags: | daniel_megert:
review+
|
||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||
| Bug Blocks: | 361821 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Ron Ratovsky
As you noted you can change the color. The overview ruler uses that color to draw the border and then uses a lightly washed out variant of that color. I guess it would be sufficient to add a new accessibility option that allows to disable the washout. That sounds like a fair and simple solution to the issue. Thanks. We'll have to mention the preference in org.eclipse.platform.doc.user/concepts/accessibility/text_editor.htm Created attachment 205393 [details]
Patch
Patch that adds a preference to Accessibility preference page to disable the color washout.
Dani , could you pls have a look at the patch? The "wash out" is more an implementation detail of our overview ruler and we don't need to make this API via interface. I would take a similar approach as in DefaultHyperlinkPresenter. This will minimize the files we have to touch. Also, "wash out" is not a good name. We should change this to something like "Use saturated color in overview ruler". Better suggestions are welcome. > I would take a similar approach as in DefaultHyperlinkPresenter. This will
> minimize the files we have to touch.
Even better is a solution which is closer to the patch but doesn't touch the SourceViewerDecorationSupport and ui.workbench.texteditor at all.
Created attachment 205503 [details] Patch_v2 (In reply to comment #7) > > I would take a similar approach as in DefaultHyperlinkPresenter. This will > > minimize the files we have to touch. > > Even better is a solution which is closer to the patch but doesn't touch the > SourceViewerDecorationSupport and ui.workbench.texteditor at all. I agree. I didn't like the approach to pass the preference store to OverviewRuler too much, that's why I went with adding new API to the IOverviewRulerExtension. However as discussed, its better to not pollute SourceViewerDecorationSupport instead implement the same in AbstractDecoratedTextEditor itself. (In reply to comment #6) > Also, "wash out" is not a good name. We should change this to something like > "Use saturated color in overview ruler". Better suggestions are welcome. I went with the above suggested as I could not come up with a better name. The patch has several issues: - When applying it says there is an error/conflict on the new file. I tried whether EGit correctly handles new files when creating and applying a patch and it does. So, I guess you made something wrong when creating the patch. - It causes API Tools errors. Please make sure to have API Tools setup correctly and fix all the issues. - When adding an extension interface you also have to add a link to the extension from the original interface (see e.g. ITextViewer). - I don't like 'setUseSaturatedColorInOverviewRuler(...). We don't need to call update during creation and we don't need the null check. Inline that code. - The new code in handlePreferenceStoreChanged should be close to the other code that deals with the overview ruler. - Mention IOverviewRulerExtension.setUseSaturatedColorPreference(boolean) when declaring the constant, since the preference only works when the ruler implements that interface. - Doc update for the preference page is missing. Created attachment 205702 [details] Patch_v3 (In reply to comment #9) > The patch has several issues: > > - When applying it says there is an error/conflict on the new file. I tried > whether EGit correctly handles new files when creating and applying a patch > and it does. So, I guess you made something wrong when creating the patch. This time I directly copied the diff from the history view (build commit info) , pls let me know if this is ok. Fixed the rest of the issues. > - Doc update for the preference page is missing. I still have to do this. > > - When applying it says there is an error/conflict on the new file. I tried
> > whether EGit correctly handles new files when creating and applying a patch
> > and it does. So, I guess you made something wrong when creating the patch.
>
> This time I directly copied the diff from the history view (build commit info)
> , pls let me know if this is ok. Fixed the rest of the issues.
>
No, it does not work (even more conflicting files). I also let Deepak try it - no luck. No clue what you're doing wrong. You don't need to copy anything. Simply make the changes, commit, then in the History view use context menu > Create Patch... Of course you need to make sure that there are no additional commits between the selected one and what's currently in 'master. Also, it's safer to write the patch to a file instead of going via clipboard.
Created attachment 205707 [details]
Patch_v3
I'm not sure why there were conflicts in the new file, I see the delimiters are fine (unix) in the editor for all files.
Created patch via 'Create patch...' from History view. I tried doing this earlier but the line delims did not appear in notepad properly and hence I tried to copy the diff from History view to make the patch. However this time I created a patch in different format and then converted it into a text file (which seemed to have changed the new file mode). It should work fine now.
The patch also does not work. Please verify the next patch against a clean master before attaching it. I'm tired of trying broken patches. (In reply to comment #14) > http://people.gnome.org/~michael/data/2011-10-13-new-developers.pdf Sure. (In reply to comment #12) > Created attachment 205707 [details] > Patch_v3 Raksha, which build of EGit are you using right now? (In reply to comment #16) > (In reply to comment #12) > > Created attachment 205707 [details] [details] > > Patch_v3 > > Raksha, which build of EGit are you using right now? I'm using an old version Eclipse EGit 1.0.0.201106090707-r org.eclipse.egit.feature.group Eclipse EGit (In reply to comment #17) > I'm using an old version > Eclipse EGit 1.0.0.201106090707-r org.eclipse.egit.feature.group > Eclipse EGit That would certainly explain why your attachment 205707 [details] looks like that. Your attachment 205503 [details] _looks_ better but I guess there were still problems according to comment 9. Benny made some changes in August (66cf6e8995b6e2338e8fd1dddf0cd751d2169e2e) which removed a few annoying problems with regarding to patches generated from EGit. I would suggest you upgrade to EGit 1.1 if possible. Please do not hesitate to contact me on IRC or IM if you need assistance, Raksha. Created attachment 205747 [details] Patch_v4 (In reply to comment #18) > Benny made some changes in August (66cf6e8995b6e2338e8fd1dddf0cd751d2169e2e) > which removed a few annoying problems with regarding to patches generated from > EGit. I would suggest you upgrade to EGit 1.1 if possible. Yep, finally problem solved by upgrading the Egit to latest and also setting 'Ignore leading path name segments' to 1. Thanks for your help Remy. (In reply to comment #13) > The patch also does not work. Please verify the next patch against a clean > master before attaching it. I'm tired of trying broken patches. Here's another one Dani ;) Pls set the 'Ignore leading path name segments' to 1 when you apply. > Here's another one Dani ;) Pls set the 'Ignore leading path name segments' to 1
> when you apply.
Did you try it? I still get an conflict, see attached picture.
Created attachment 205748 [details]
Picture showing conflict
(In reply to comment #20) > > Here's another one Dani ;) Pls set the 'Ignore leading path name segments' to 1 > > when you apply. > > Did you try it? I still get an conflict, see attached picture. Yep I tried a few times. It works fine for me. Only that the AbstractDecoratedTextEditorPreferenceConstants has picked up some noise somehow :( . I will attach a patch for that again. Markus, please try the latest patch. Raksha says it works for her, but it does not apply for me. > Markus, please try the latest patch. Raksha says it works for her, but it does
> not apply for me.
Tricky: The patch contains diffs for files from multiple projects, but is not a proper Eclipse workspace patch (with comments that tell about the project).
The Apply Patch wizard asks for a project, but if you go that route, you always get errors. Instead, you have to apply to the workspace root. Then it works for me.
Created attachment 205750 [details]
Patch_v4.5
Removed noise changes from AbstractDecoratedTextEditorPreferenceConstants.java .
Aaah! Figured it out now and the patch applies now. One of the previous patches added a new file (overview ruler extension) but Hard Reset did not remove that file (filed bug 361696) and hence the conflict. Sorry Raksha for the trouble! (In reply to comment #26) > Hard Reset did not remove that file (filed bug 361696) Mmmh, actually Git reset is supposed to not remove the untracked files. One does this with the git clean command which unfortunately is not implemented by EGit (see bug 338717). However, the real culprit is that EGit does not indicate that a project has (outgoing) changes due to some untracked files inside the project (see bug 359264). So, the lesson I learned is that if something looks wrong/fishy always first distrust EGit and double-check on the Git command line. We could have saved time on both sides if I'd done that. Raksha, I apologize once again for the trouble this caused. The patch only partially works: when I toggle the preference it does not update in the ruler unless I force a redraw manually. You need to call fOverviewRuler.update().
Some minor issues:
- The OverviewRuler.setUseSaturatedColorPreference(boolean) should not have its
own Javadoc. Currently it even gives a Javadoc warning.
- We should say something about the default value for the saturated color pref:
- In the new interface I would say that it is up to the ruler implementation
to specify the initial default unless setUseSaturatedColorPreference(...)
is called.
- In the 'OverviewRuler' class Javadoc I would mention that it uses non-
saturated colors unless setUseSaturatedColorPreference(...) gets called.
- I mentioned it before but maybe I should have been more clear: when adding an
extension interface you also have to add a link to the extension from the
original interface. With "link" I not only meant to add the @see tag but also
document it a bit more, like e.g. in the ITextHover:
* <p>
*
* In order to provide backward compatibility for clients of
* <code>ITextHover</code>, extension interfaces are used as a means of
* evolution. The following extension interfaces exist:
* <ul>
* <li>{@link org.eclipse.jface.text.ITextHoverExtension} since version 3.0
* allowing a text hover to provide a creator for the hover control. This
* allows
* for sophisticated hovers in a way that information computed by the hover
* can
* be displayed in the best possible form.</li>
* </ul></p>
- Some of the files have superfluous whitespace (e.g. tabs) at the end some
lines, e.g. in OverviewRuler.setUseSaturatedColorPreference(boolean).
Bonus: you could add "ruler" as preference keyword, so that the 'Accessibility' page is listed when entering "ruler" into the search field.
> - The OverviewRuler.setUseSaturatedColorPreference(boolean) should not have its
> own Javadoc. Currently it even gives a Javadoc warning.
Correction: it needs at least the @since tag because this won't be inherited. To avoid the Javadoc warning you can write:
/**
* {@inheritDoc}
*
* @since 3.8
*/
Created attachment 205785 [details] Patch_v5 (In reply to comment #28) > The patch only partially works: when I toggle the preference it does not update > in the ruler unless I force a redraw manually. You need to call > fOverviewRuler.update(). Ah I only meant to remove it at createPartControl(..), but accidentally removed it in handlePreferenceStoreChanged(..). Added it back. > > Some minor issues: > Fixed the javadoc. > Bonus: you could add "ruler" as preference keyword, so that the 'Accessibility' > page is listed when entering "ruler" into the search field. Done. The works fine now Raksha. I've pushed it with some minor modifications - removed most occurrences of "preference" from the API and its Javadoc - changed "color" to "colors" since there's not just one color - some minor tweaks in the Javadocs - changed required version of jface.text to 3.8 in the manifest of ui.editors. to master: e7881ae9494ff0db8f6f0dd7dfbab8fb42ed24ab (In reply to comment #31) > The works fine now Raksha. I've pushed it with some minor modifications > > - removed most occurrences of "preference" from the API and its Javadoc > - changed "color" to "colors" since there's not just one color > - some minor tweaks in the Javadocs > - changed required version of jface.text to 3.8 in the manifest of ui.editors. > > to master: e7881ae9494ff0db8f6f0dd7dfbab8fb42ed24ab Thanks. I see some noise in the javadoc (ex : IOverviewRuler) , I guess its intentional. > Thanks. I see some noise in the javadoc (ex : IOverviewRuler) , I guess its
> intentional.
Yes, the save action formatted the Javadoc according to our formatting rules.
> > - Doc update for the preference page is missing. > > I still have to do this. This is tracked by bug 361821. Verified in Plug-in Export I20111021-0800.from20111025_1442. |