Bugzilla – Full Text Bug Listing
| Summary: | provide a dialog for selecting Git change sets | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Sebastien Dubois <sebastien.dubois> | ||||||||||||||
| Component: | Mylyn | Assignee: | Sebastien Dubois <sebastien.dubois> | ||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||
| Severity: | enhancement | ||||||||||||||||
| Priority: | P3 | CC: | alvaro.sanchez-leon, lmcbout, steffen.pingel | ||||||||||||||
| Version: | unspecified | Keywords: | contributed | ||||||||||||||
| Target Milestone: | 0.8 | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | Windows Vista | ||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Bug Depends on: | |||||||||||||||||
| Bug Blocks: | 346462 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
Sebastien, can you describe what the attached patch does? This is a the Git UI Connector, which offers the possibility to select a Commit, resolve the changeset and pass it on to a user application. The selection and the presentation will likely evolve to more sophisticated selections and include project filters, etc... I have modified it a bit to reflect internal packages, and copyright formatting, I have committed the plug-in under Mylyn Versions HEAD. I noticed a few more things that should be changed: * Following our general convention, Activator should be called GitUiPlugin. It should not extend AbstractUIPlugin and be registred as a bundle activator unless it is actually required for the plug-in life-cycle. * Copyright headers need to use the exact formatting that is present in the code template. * The Manifest name and provider need to present following the standard conventions. * Manifest header values need to be localized. * The .settings folder needs to contain the standard settings files present in all plug-ins. * Source code needs to be formatted according to the standard rules * Version constraints should allow usage with Eclipse 3.5.0 as the minimum (not 3.5.2) * It's critical that each plug-in has an about.html file. This is currently missing. It's generally easiest to copy an existing plug-in when creating a new one: http://wiki.eclipse.org/Mylyn/Contributor_Reference#Creating_new_plug-ins . Should the o.e.m.git.ui plug-in be added to build and the git connector feature? Alvaro, when accepting contributions please ensure that the corresponding attachment is properly flagged as IPlog+. Attachement is now marked IPLog+ Sebastien, please update as per the comments from Steffen. Let's have a good reading on the Mylyn contributor guide. Created attachment 193389 [details]
updated git ui contribution
I see you have included a new version of the plug-in, since the initial contribution is already in the repo. it's best to attach a patch so the deltas are captured and can be checked more easily. If the additions include new files, you can zip the patch together with the additonal files. Created attachment 193902 [details]
Git ui patch 22/04/2011
The patch was missing, a) added to the build e.g. added a pom.xml for the ui plug-in, and include the git.ui plug-in in the parent pom.xml b) added o.e.m.git.ui to the git connector feature I modified the bundle name from: Mylyn Git Connector UI (Incubation) to: Mylyn Git UI Connector (Incubation) I have committed the changes and mark the patch as Iplog Can you please make the following additional changes: * Move the "Description" part from the copyright header to the JavaDoc comment of the class. * Remove the // Constants // Methods etc. comments. * Remove comments that do not add any meaningful description, e.g. "Method getChangeSet." or * The code formatter template is not the standard Mylyn template. Please use the correct one and reformat the sources. * Following standard Mylyn conventions ScmUiConnector should be named ScmConnectorUi * Please follow the project convention of putting "null" on the right hand side of comparisons, e.g. if (null == resource) should be replaced by if (resource == null) * When checking pre-conditions use Assert.isNotNull(resource) instead of if (resource == null)... * Please remove all $Revision$ tags. That is CVS specific and generally only causes troube and should never be used in the @version tag. The @version tag specifies the API version of a file and not the revision (which is available from the history). Only classes/methods that are API should have an @version tag. * Why does the GitUiConnector reference Clear Case in comments? * Please remove commets that are inacurate, e.g.: /** * Field ADD_ANOMALY_DIALOG_TITLE. (value is ""Enter Anomaly details"") */ private static final String FIND_REVIEW_ITEMS_DIALOG_TITLE = "Find Review Items"; * Please do not prefix class fields with "f", e.g. use repositoryNameText instead of fRepositoryNameText. * Please use SelectionAdapter instead of new SelectionListener unless all methods are implemented. * Please use proper camel casing, e.g. authorNameLabel instead of authorNamelabel. * Never pass "null" for an IProgressMonitor when running background operations. Sebastien, I know that's a lot of change requests and I wish we would have done a more thorough review before merging the contribution but I would like to ask you to provide another patch that addresses the problems above. It's very important that we use common coding conventions to facilitate the collaboration model that has helped the project to grow in the past. (In reply to comment #9) > Can you please make the following additional changes: > * Move the "Description" part from the copyright header to the JavaDoc comment > of the class. > * Remove the // Constants // Methods etc. comments. > * Remove comments that do not add any meaningful description, e.g. "Method > getChangeSet." or > * The code formatter template is not the standard Mylyn template. Please use > the correct one and reformat the sources. > * Following standard Mylyn conventions ScmUiConnector should be named > ScmConnectorUi > * Please follow the project convention of putting "null" on the right hand side > of comparisons, e.g. if (null == resource) should be replaced by if (resource > == null) > * When checking pre-conditions use Assert.isNotNull(resource) instead of if > (resource == null)... > * Please remove all $Revision$ tags. That is CVS specific and generally only > causes troube and should never be used in the @version tag. The @version tag > specifies the API version of a file and not the revision (which is available > from the history). Only classes/methods that are API should have an @version > tag. > * Why does the GitUiConnector reference Clear Case in comments? > * Please remove commets that are inacurate, e.g.: > /** > * Field ADD_ANOMALY_DIALOG_TITLE. (value is ""Enter Anomaly details"") > */ > private static final String FIND_REVIEW_ITEMS_DIALOG_TITLE = "Find Review > Items"; > * Please do not prefix class fields with "f", e.g. use repositoryNameText > instead of fRepositoryNameText. > * Please use SelectionAdapter instead of new SelectionListener unless all > methods are implemented. > * Please use proper camel casing, e.g. authorNameLabel instead of > authorNamelabel. > * Never pass "null" for an IProgressMonitor when running background operations. > > Sebastien, I know that's a lot of change requests and I wish we would have done > a more thorough review before merging the contribution but I would like to ask > you to provide another patch that addresses the problems above. It's very > important that we use common coding conventions to facilitate the collaboration > model that has helped the project to grow in the past. No problem. I will implement the changes and provide a new patch ASAP. Thanks for the guidance. Additional changes implemented as per Steffen's comments. The changes are included in the patches for bug 344982. Please refer to this bug for follow up. I will close this one *** This bug has been marked as a duplicate of bug 344982 *** Re-opening, The bug is not really a duplicate of Bug 344982. However you are using one patch for both issues. It's best to have separate patches whenever possible. Created attachment 195334 [details]
updated patch
Created attachment 195338 [details] updated comment 9 I have checked the modifications in the patches (2011-05-11) and they seem ok to me. I have accepted and committed the patches to recover the nightly build. since the patches are the same as submitted in bug 344982, I will only mark them as iplog on this one. Thanks for making these changes! There are still a number of uses of NullProgressMonitor that need to be addressed. Could you post a screen shot so that we can do a UI review of the dialog? Thanks! Alvaro, you have to mark the patches that were provided by the contributor as iplog+ otherwise they do not get tracked in the iplog properly. Please correct that. Sebastien, just on a minor note, please do not mark bugs as duplicates if work has already been completed. It's better to have multiple resolved bugs even if changes are related. Otherwise it becomes more difficult to track what changed were made for a particular milestone and you get less credit :). > Alvaro, you have to mark the patches that were provided by the contributor as > iplog+ otherwise they do not get tracked in the iplog properly. Please correct > that. > As requested the patches on bug 344982 as well as on this bug are now marked Iplog+ (even if they are the same) Created attachment 195862 [details]
Git UI dialog screenshot
I am marking this as resolved and have opened bug 346462 to track the remaining work. |
Created attachment 192399 [details] source code for the Git connector UI plugin This is a new contribution for Mylyn version Git UI connector. I'm writing this as a bog for now since I do not have committer rights (yet). Legal Message: I, Sebastien Dubois, declare that I developed attached code from scratch, without referencing any 3rd party materials except material licensed under the EPL. {I am authorized by my employer to make this contribution under the EPL.}