Community
Participate
Working Groups
I issued again a GSoC project to generify JFace. Last year the student was fighting with losts of whitespace changes missing Overrides etc. To avoid loosing again lots of efficiency I suggest to clean the code up once so that he can work efficiently.
We don't do random white space changes in the code base (that's why we don't run general clean up on the code, it issues a "most lines have changed even if they haven't). jface should have the "format only lines that change" save action enabled, however. PW
Jeanderson, can you provide a patch to remove all (Non-Javadoc) in the viewers package?
(In reply to Lars Vogel from comment #2) > Jeanderson, can you provide a patch to remove all (Non-Javadoc) in the > viewers package? Hi Lars, Just a clarification: If a method has javadoc and it has an @Override annotation, is it okay to remove it too or only (Non-Javadoc) should be removed (in the first case, it would not be removed)? Thanks in advance!
(In reply to Jeanderson Candido from comment #3) > Just a clarification: If a method has javadoc and it has an @Override > annotation, is it okay to remove it too or only (Non-Javadoc) should be > removed (in the first case, it would not be removed)? > > Thanks in advance! I would leave normal Javadoc in, except it is just a copy of the Javadoc from the super method. If in doubt, leave it in, you don't have to check all the super Javadoc.
(In reply to Lars Vogel from comment #4) > (In reply to Jeanderson Candido from comment #3) > > Just a clarification: If a method has javadoc and it has an @Override > > annotation, is it okay to remove it too or only (Non-Javadoc) should be > > removed (in the first case, it would not be removed)? > > > > Thanks in advance! > > I would leave normal Javadoc in, except it is just a copy of the Javadoc > from the super method. If in doubt, leave it in, you don't have to check all > the super Javadoc. That's what I thought... Some comments are like an "specialization" of the super Javadoc and maybe it would not be good to remove them. Here is my review: https://git.eclipse.org/r/#/c/23821/ A few notes: - I'm using 4.4M6 (so far, a solid rock on Windows) and apparently (I don't recall pressing CRTL+SHIFT+F) it automatically organizes the imports: Before: import org.eclipse.swt.graphics.*; After: import org.eclipse.swt.graphics.Image; import org.eclipse.swt.graphics.ImageData; import org.eclipse.swt.graphics.Point; ..And it also organizes imports by lexical order. I just realized it when I was looking on Gerrit and I'm aware that I may have to undo this imports. Finally, it was a large commit. I changed all files from jface.viewers.* and maybe it would be better to break it into small commits if necessary. Thanks Lars and Paul. I added Lars and Paul to review it. Thanks in advance.
Jeandersons cleanup applied with https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=fb75e64d2e700c0d6e76b188c5c81746547dbec6
Enhanced for loop for TreeViewer https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=7cf08fd497950791e3594c268bc04f9df8434a4e
More Viewers done with https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=960baf804d1f0cfba758954965b196db243fecb1
Deprecated all public entities of TableTreeViewer: https://git.eclipse.org/r/23905
Jeanderson added @Deprecated to all public fields and methods of TableTreeViewer https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=f0639997c1f6a2fab1fa1fc9f4615b8a02b00fdf
You've deleted these API Javadocs: org.eclipse.jface.viewers.AbstractListViewer#setLabelProvider(IBaseLabelProvider) org.eclipse.jface.viewers.StyledString#toString() ... and this interesting comment: org.eclipse.jface.viewers.AbstractTreeViewer#handleDoubleSelect(SelectionEvent) Please restore.
(In reply to Markus Keller from comment #11) > You've deleted these API Javadocs: > org.eclipse.jface.viewers. > AbstractListViewer#setLabelProvider(IBaseLabelProvider) > org.eclipse.jface.viewers.StyledString#toString() > > ... and this interesting comment: > org.eclipse.jface.viewers. > AbstractTreeViewer#handleDoubleSelect(SelectionEvent) > > Please restore. Thanks Markus, restored with https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=534a4ce8282ebf762fb1102ba5acfb21fdd2709a
Using foreach loop in ComboViewer, TableViewer and Viewer https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=36c8bf2418a7b38c1115a58e91b85546c1ce7d58
Enhanced for loop for TreeViewer https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=beaeda9df8e02d0bc032b5dd0f20414ec6d49f92 I think I mark this bug as fixed for now, code looks much nicer to me already. Hopefully this will make the GSoC a bit smoother.
(In reply to Lars Vogel from comment #13) > Using foreach loop in ComboViewer, TableViewer and Viewer > https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=36c8bf2418a7b38c1115a58e91b85546c1ce7d58 Sorry that one was https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=4e61992bddbcc4508556d441a50e9f18fcd7f9a3
I simplified some loops from ToolBarManager using "for each" sintax and removed some temporary variables. In addition, the code had spaces and tabs mixed. I fixed that too. Review: https://git.eclipse.org/r/#/c/24162/2
(In reply to Lars Vogel from comment #14) > Enhanced for loop for TreeViewer > https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=beaeda9df8e02d0bc032b5dd0f20414ec6d49f92 > > I think I mark this bug as fixed for now, code looks much nicer to me > already. Hopefully this will make the GSoC a bit smoother. Lars, is there any other clean up that I should do? We could use this thread to for gerrit reviews related to cleaning up JFace code. I just sent a review for another class. Indeed, the code looks much better now. Thanks! JBC
(In reply to Jeanderson Candido from comment #16) > I simplified some loops from ToolBarManager using "for each" sintax and > removed some temporary variables. In addition, the code had spaces and tabs > mixed. I fixed that too. > > Review: https://git.eclipse.org/r/#/c/24162/2 Applied with https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=a8d443f68d0a87ac1d5922ce76d86bf8d99165f4
Started to clean up JFace Test Suite code: https://git.eclipse.org/r/#/c/25602/ Should we keep it in this issue or open a file a new bug for JFace tests?
Unnecessary comments in AbstractFieldAssistTestCase: https://git.eclipse.org/r/25616
More cleanup: https://git.eclipse.org/r/#/c/25618/
(In reply to Jeanderson Candido from comment #19) > Started to clean up JFace Test Suite code: > https://git.eclipse.org/r/#/c/25602/ > Should we keep it in this issue or open a file a new bug for JFace tests? I think a new bug would be good for future Gerrit review. (Clean JFace Unit tests). I handle the existing Gerrit reviews via this bug.
Test clean-ups applied with https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=1cc4022ce3bdef3bde83b76e725cd51b04f07d2c https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=e50354c09431846715749bc8ab4812e636c25703 https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=5225722a5a5012d68c424d7cc56f6a2145db7b0c
(In reply to Lars Vogel from comment #23) > Test clean-ups applied with > https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=1cc4022ce3bdef3bde83b76e725cd51b04f07d2c > https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=e50354c09431846715749bc8ab4812e636c25703 > https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=5225722a5a5012d68c424d7cc56f6a2145db7b0c Thank you Lars! By the way, I'm not sure but I guess you forgot to merge two verified patches: 1. https://git.eclipse.org/r/#/c/25616/ 2. https://git.eclipse.org/r/#/c/25602/ Thanks in advance.
(In reply to Jeanderson Candido from comment #24) > Thank you Lars! By the way, I'm not sure but I guess you forgot to merge two > verified patches: > > 1. https://git.eclipse.org/r/#/c/25616/ > 2. https://git.eclipse.org/r/#/c/25602/ > > Thanks in advance. Merged with https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=eb715d5602d518231277f645e3217faa73159fee https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=25d51df514902194702b3b017ba6967788befc09 Thanks
Verified in Git
