Community
Participate
Working Groups
We would like to propose the Gerrit Dashboard to be associated to the Gerrit Connector. In attachment, we put some screen snapshot of what it looks like. We can explain and give a demo at the next Mylyn meeting.
Created attachment 236104 [details] Initial screen for the Gerrit Dashboard Show the Gerrit Dashboard and associated data
Created attachment 236105 [details] Gerrit server selection Menu showing the selection of one of the Gerrit server
Created attachment 236106 [details] All menu selection pulldown Showing the All menu selection from the Dashboard
Created attachment 236107 [details] Document selection pulldown Showing the Documentation menu selection from the Dashboard
Created attachment 236108 [details] My menu pulldown selection Showing the MY menu selection from the Dashboard
Thanks for doing the demo, this looks great! I have a few suggestions: * At the top of the dashboard view there is a big grey bar to the left of the All and My menus (it's not in the screenshot but it was in the demo). Would it make sense to move the repository and version information there so that there would only be a single line of controls between that bar and the review table? We could remove the "Repository:" and "Version:" labels so that it would just be like a title for the view, e.g. "Eclipse.org Reviews - Gerrit 2.6.1" and use a title font. ** Maybe version is not important enough to include in the title. I wonder if it should be displayed at all. * As Steffen suggested, the ITasksCoreConstants.ATTRIBUTE_HIDDEN attribute should be set on the queries created by the dashboard * I'm still not sold on the need to add a button to the main toolbar, since most users would probably only use it once when adding their first Gerrit repository and there is limited toolbar space available. If we do that though, the add menu item should have a more descriptive label like "Add Gerrit Repository..." because currently users who don't recognize the icon would have no idea what it does. * If we merge this into Mylyn Reviews, we should revisit adding a separate review perspective (bug 390372) and consider what layout options we have for the views. This would also lessen even further the need for a button on the main toolbar.
Thanks very much for the great demo today. Here are my suggestions that I mentioned on the call: * Change the Add... button in the drop down to show a blank repository settings dialog which only supports adding repositories but not editing of existing repositories. * To support editing (which should be a rare activity) I would add a "Show Team Repositories" menu item that opens the Team Repositories view. This is consistent with other Mylyn views. * The UI should be using the new "Diffy" icon. * Consider naming the view "Gerrit Reviews" or "Gerrit Dashboard" to make it clear that it's specific to Gerrit reviews. * We should discuss the best entry point for the dashboard. I agree with Sam that we have to be very careful adding icons to the main toolbar. With many plug-ins installed Eclipse quickly becomes overloaded with UI contributions and the always visible main toolbar is particularly valuable screen estate that should be reserved for frequently used UI affordances. Something that could be added later would be a history of searches in the search text field, e.g. through a drop down. That would make it easy to switch between multiple custom queries.
I saw the demo earlier, and I was also really happy w/ it. I'll just throw in a few extra thoughts.. (In reply to comment #6) > Thanks for doing the demo, this looks great! I have a few suggestions: > > * At the top of the dashboard view there is a big grey bar to the left of the > All and My menus (it's not in the screenshot but it was in the demo). Would it > make sense to move the repository and version information there so that there > would only be a single line of controls between that bar and the review table? > We could remove the "Repository:" and "Version:" labels so that it would just be > like a title for the view, e.g. "Eclipse.org Reviews - Gerrit 2.6.1" and use a > title font. +1 Overall, I'm not sure we need all of that header information. I'd prefer to keep it really simply and say Eclipse.org Reviews and my watched changes. Also, I wonder if the "All" "My" menus shouldn't simply be associated with the entry area for the search, since that's where they would end up..? Also, the "Documentation" button doesn't seem inline w/ Eclipse HIG, even though I like the general idea. Perhaps just a "?" icon? > * I'm still not sold on the need to add a button to the main toolbar, since most > users would probably only use it once when adding their first Gerrit repository > and there is limited toolbar space available. If we do that though, the add menu > item should have a more descriptive label like "Add Gerrit Repository..." > because currently users who don't recognize the icon would have no idea what it > does. I thing Add.. is just fine as the context is pretty clear. > * If we merge this into Mylyn Reviews, we should revisit adding a separate > review perspective (bug 390372) and consider what layout options we have for the > views. This would also lessen even further the need for a button on the main > toolbar. +1 for having it appear only on the Reviews perspective, and obviously I'm +1 on seeing that revisited. :) (In reply to comment #7) > Thanks very much for the great demo today. Here are my suggestions that I > mentioned on the call: > > * Change the Add... button in the drop down to show a blank repository settings > dialog which only supports adding repositories but not editing of existing > repositories. Sorry, what is it showing now? > * To support editing (which should be a rare activity) I would add a "Show Team > Repositories" menu item that opens the Team Repositories view. This is > consistent with other Mylyn views. Can we get a creeenshot of what it's showing now? > * Consider naming the view "Gerrit Reviews" or "Gerrit Dashboard" to make it > clear that it's specific to Gerrit reviews. +1. Needs to refer to Gerrit, even Reviews perspective might support other review types. "Gerrit Dashboard" is a bit awkward though. Note we already have a "Review" view, though that could also be renamed. Perhaps "Gerrit Reviews". > * We should discuss the best entry point for the dashboard. I agree with Sam > that we have to be very careful adding icons to the main toolbar. With many > plug-ins installed Eclipse quickly becomes overloaded with UI contributions and > the always visible main toolbar is particularly valuable screen estate that > should be reserved for frequently used UI affordances. Yeah, I don't think we should have this on the main Eclipse toolbar. > Something that could be added later would be a history of searches in the search > text field, e.g. through a drop down. That would make it easy to switch between > multiple custom queries. Would be nice. Also, completion for common search predicates! I find the google'ish search query in web ui to be a bit opaque as I'm never quite sure what keys to enter. A mini-search editor would be nice to have eventually.
(In reply to comment #6) > Thanks for doing the demo, this looks great! I have a few suggestions: > > * At the top of the dashboard view there is a big grey bar to the left of the > All and My menus (it's not in the screenshot but it was in the demo). Would it > make sense to move the repository and version information there so that there > would only be a single line of controls between that bar and the review table? The command bar moves automatically beside the window title when there is enough spaces for it. It seems also to depend of the eclipse version. Using eclipse 3.8 is using a lot of spaces, but eclipse 4.3, it moves along the window title. When the move occurs, then the extra spaces used is gone. > We could remove the "Repository:" and "Version:" labels so that it would just be > like a title for the view, e.g. "Eclipse.org Reviews - Gerrit 2.6.1" and use a > title font. As suggested, I removed the two labels and replace it with just the data. For ex: "Eclipse.org Reviews - Gerrit 2.6.1" By doing this, I reduced the size of the box to a single line, so more space for the list of reviews. > ** Maybe version is not important enough to include in the title. I wonder if it > should be displayed at all. When switching from one Gerrit to another, the version becomes important. We can also see this information when using the web interface. I think it should stay. > * As Steffen suggested, the ITasksCoreConstants.ATTRIBUTE_HIDDEN attribute > should be set on the queries created by the dashboard I modified the code to use the hidden attributes. This prevent from seeing the data in the Task list and all the data showed when receiving the Task list information. It looks to be a plus. > * I'm still not sold on the need to add a button to the main toolbar, since most > users would probably only use it once when adding their first Gerrit repository > and there is limited toolbar space available. If we do that though, the add menu > item should have a more descriptive label like "Add Gerrit Repository..." > because currently users who don't recognize the icon would have no idea what it > does. Changing the name "Ad" by "Add Gerrit Repository ..." sounds good. Removing the button from the toolbar, Humm!!, Using an icon is not taking so much places and it ease the access to use the Gerrit review. The end-user does not have to open to to the eclipse menu window -> show view -> other-.... before accessing the Mylyn repository before opening the dialogue and be able to enter the definition of the Gerrit server. The button become even more important when we use the Hidden attributes, the user does not need again to use different menu to open the Task list since the data is not shown anymore. So the user have access to a Gerrit definition and the Dashboard very quickly without disrupting his current layout. I don't know about you, but may be the button should also show the Mylyn Review view simultaneously with the Dashboard, so his current working layout would have all the necessary view to perform a review . Another reason for the button, when the end-user has completed a review, he can delete the dashboard, continue his work and when ready for another review, he just select the Dashboard button in the main toolbar. It re-open the dashboard, perform a query for the "My Watched changes " by default. It becomes very efficient. > * If we merge this into Mylyn Reviews, we should revisit adding a separate > review perspective (bug 390372) and consider what layout options we have for the > views. This would also lessen even further the need for a button on the main > toolbar. I just look at the bug 390372. I am not sure about having a perspective for the review. We tried it a while ago with R4E and got rid of it. The end-user preferred to use their current working workspace . So this is why we added minimum views and use minimal space from their editor to perform reviews.
(In reply to Jacques Bouthillier from comment #9) Jacques, I can see the value in the button. I also the EGit global toolbar buttons quite often to push/pull/commit. However, they do not add the buttons by default. They have to be enabled using the customize perspective menu. Because the toolbar is already pretty cluttered the dashboard should try to be a good citizen and not add it by default globally. Also, I suggest to add a keyboard shortcut for bringing up the dashboard. That's even more efficient for keyboard folks. :)
(In reply to comment #9) > (In reply to comment #6) > > Thanks for doing the demo, this looks great! I have a few suggestions: > > > > * At the top of the dashboard view there is a big grey bar to the left of the > > All and My menus (it's not in the screenshot but it was in the demo). Would it > > make sense to move the repository and version information there so that there > > would only be a single line of controls between that bar and the review table? > The command bar moves automatically beside the window title when there is enough > spaces for it. It seems also to depend of the eclipse version. Using eclipse 3.8 > is using a lot of spaces, but eclipse 4.3, it moves along the window title. > When the move occurs, then the extra spaces used is gone. > > > We could remove the "Repository:" and "Version:" labels so that it would just > be > > like a title for the view, e.g. "Eclipse.org Reviews - Gerrit 2.6.1" and use a > > title font. > As suggested, I removed the two labels and replace it with just the data. For > ex: "Eclipse.org Reviews - Gerrit 2.6.1" > By doing this, I reduced the size of the box to a single line, so more space for > the list of reviews. > > > ** Maybe version is not important enough to include in the title. I wonder if > it > > should be displayed at all. > When switching from one Gerrit to another, the version becomes important. We > can also see this information when using the web interface. I think it should > stay. > > > * As Steffen suggested, the ITasksCoreConstants.ATTRIBUTE_HIDDEN attribute > > should be set on the queries created by the dashboard > I modified the code to use the hidden attributes. This prevent from seeing the > data in the Task list and all the data showed when receiving the Task list > information. It looks to be a plus. > > > * I'm still not sold on the need to add a button to the main toolbar, since > most > > users would probably only use it once when adding their first Gerrit > repository > > and there is limited toolbar space available. If we do that though, the add > menu > > item should have a more descriptive label like "Add Gerrit Repository..." > > because currently users who don't recognize the icon would have no idea what > it > > does. > Changing the name "Ad" by "Add Gerrit Repository ..." sounds good. Removing the > button from the toolbar, Humm!!, Using an icon is not taking so much places and ... > Dashboard button in the main toolbar. It re-open the dashboard, perform a query > for the "My Watched changes " by default. It becomes very efficient. > > > * If we merge this into Mylyn Reviews, we should revisit adding a separate > > review perspective (bug 390372) and consider what layout options we have for > the > > views. This would also lessen even further the need for a button on the main > > toolbar. > I just look at the bug 390372. I am not sure about having a perspective for the > review. We tried it a while ago with R4E and got rid of it. The end-user > preferred to use their current working workspace . So this is why we added > minimum views and use minimal space from their editor to perform reviews. Hmm.. I'd tend to defer to you on user acceptance as you guys have a lot of experience w/ what users actually use. And users are strangely averse to perspectives. :D S I see the point -- it sounds like it's really more of a single Review toolbar button *vs.* a new perspective rather than both and in the case of not having a perspective, I think having it on the toolbar might make sense. After all, the user can remove it if they don't like the addition. I for one would like to have it, but I never use the toolbar for quick affordances anyway.. I also really like the idea of providing a way to get the "Reviews" view opening when the Gerrit Review is shown. That would provide a solution for bug 409418 which we need if we ever want anyone to actually use that. In fact, I've even thought about the idea of combining the two into a single view..where the review has a sort of master-detail relationship w/ the review details contents but that might require too much screen real estate.
Created attachment 236564 [details] Initial eclipse view with some adjustment for the review Initial screen to perform a review within eclipse
Created attachment 236565 [details] Gerrit pulldown menu having the "Add" button change "Add" button replace with "Add Gerrit Repository..."
(In reply to comment #7) > Thanks very much for the great demo today. Here are my suggestions that I > mentioned on the call: > > * Change the Add... button in the drop down to show a blank repository settings > dialog which only supports adding repositories but not editing of existing > repositories. We will put Empty repositories when selecting the "Add.." Also, we will rename the "Add" with "Add Gerrit Repository..." > * To support editing (which should be a rare activity) I would add a "Show Team > Repositories" menu item that opens the Team Repositories view. This is > consistent with other Mylyn views. Not completed yet, but are looking to include it > * The UI should be using the new "Diffy" icon. I think the current Gerrit icon is well known already, so should we change it to the DIFFY icon ? > * Consider naming the view "Gerrit Reviews" or "Gerrit Dashboard" to make it > clear that it's specific to Gerrit reviews. Having the icon and Dashboard , should it be enough? Using "Gerrit Dashboard" is a long name, but we are ok to rename the view if you consider to be best. > * We should discuss the best entry point for the dashboard. I agree with Sam > that we have to be very careful adding icons to the main toolbar. With many > plug-ins installed Eclipse quickly becomes overloaded with UI contributions and > the always visible main toolbar is particularly valuable screen estate that > should be reserved for frequently used UI affordances. If a user only open the Dashboard to perform a review, then close it to have maximum space for his work, I think the button is well deserving his entry in the eclipse toolbar menu. This prevent many clicks to access the Review functionality. > > Something that could be added later would be a history of searches in the search > text field, e.g. through a drop down. That would make it easy to switch between > multiple custom queries. Look at the new picture, we already work on it and we also think this is a very good idea. Another thing. By using the HIDDEN attributes, no query will end-up in the task list view, so the end user can even close it and use his whole workspace to display the review at the same time as its editor. It mean more estate for is working environment.
(In reply to comment #10) > (In reply to Jacques Bouthillier from comment #9) > > Jacques, I can see the value in the button. I also the EGit global toolbar > buttons quite often to push/pull/commit. However, they do not add the buttons by > default. They have to be enabled using the customize perspective menu. Because > the toolbar is already pretty cluttered the dashboard should try to be a good > citizen and not add it by default globally. > > Also, I suggest to add a keyboard shortcut for bringing up the dashboard. That's > even more efficient for keyboard folks. :) I see your point not cluttering the eclipse toolbar with any icons. I think the eclipse toolbar should be populated with icon the end-user uses often. In this case, Performing a review can become easier if the user only select a click instead of going through a set of clicks before opening the Gerrit Dashboard to perform a review. If the end-user is including this plug-in, it means he will perform reviews on a good basis otherwise, he will not install that plug-in at all. I will discuss internally here and get more opinions on that matters again, but up to now, people who saw the Dashboard were looking for that button in the toolbar ! Lets see what the majority have to say and go with the majority here.
Thanks for making those changes. I can see that there could be value in the button but I agree with Gunnar that it might be better if users have to enable it if they want it. Personally, I keep open any view that I use more than very rarely and just minimize them when I'm not using them.
(In reply to comment #16) > Thanks for making those changes. I can see that there could be value in the > button but I agree with Gunnar that it might be better if users have to enable > it if they want it. IMO, you might as well ditch it then. The chances of most users actually customizing their toolbar to show the gerrit affordance are close to nil. :) > Personally, I keep open any view that I use more than very > rarely and just minimize them when I'm not using them. I don't minimize mine, I usually stack them. So it would be nice to have them brought to the front.(In reply to comment #14) > (In reply to comment #7) > > * The UI should be using the new "Diffy" icon. > I think the current Gerrit icon is well known already, so should we change it > to the DIFFY icon ? Definitely. We've already changed it elsewhere and we should be consistent.
I used the Diffy icon from Mylyn and add the option about the Mylyn "Task repository" in the pull-down menu
- We presented the current version of the Gerrit Dashboard to internal eclipse users. We also had a discussion on how they would like to perform a review within eclipse: - One comment was to used the button in the eclipse toolbar to initiate a review. When you install the review plugin, you plan to perform some reviews. As a suggestion, they said that if you don't want to see the Gerrit review functionality defined in the eclipse toolbar, just use Eclipse menubar -> Window -> Customize perspective and de-activate the button otherwise, the button should be available. - Another question was if they would prefer working in their known environment and add the necessary views to perform the review (Gerrrit Dashboard, Task editor, compare editor and Review view) or used those selected views within a new review perspective. The answer was that they prefer using the new views within their current perspective (Ways of working), but it would not hurt to have a perspective for some other users. - They noticed that to open the Review view, they had to go: Eclipse toolbar -> Window -> Show view -> Other -> Mylyn -> Review. A suggestion was to initiate the Review view when opening the Task editor. Some review comments are available at that time if someone else has performed a review. No need to go to a long path to initiate the Review view.
Thanks Jacques, I'll have a look at the reviews soon. I think the goal must be to get this out soon in some kind of technical preview so that we can collect more feedback. I do understand the importance of usability and a toolbar button seems like an easy thing here because it really is so convenient. However, I just like you to be aware of the IDE working group initiative. They might put a strong eye on things like this in some future if you want to be part of the downloadable Eclipse packages. Anyway, let's not care about that level of detail for now and prepare a technical review. For this to happen, I suggest that the work goes into a branch for now which makes it easier to work on than in reviews only. I'll need verify what else we need from an IP perspective but hopefully that's not too much.
(In reply to comment #20) > For this to happen, I suggest that the work goes into a branch for now which > makes it easier to work on than in reviews only. I'll need verify what else we > need from an IP perspective but hopefully that's not too much. Branches add additional mangement burden since we would need extra Hudson build jobs, repositories etc. I would suggest to keep the dashboard in a separate bundle so we can publish it on the nightly site for evaluation. Once it's ready we can add it to the main distribution.
*** Bug 424487 has been marked as a duplicate of this bug. ***
Code push and ready for a review https://git.eclipse.org/r/#/c/20073/
As requested to define the % of people contribution for the Gerrit Dashboard Contribution of the Dashboard: Dashboard.core: Francois Chouinard: 75% Jacques Bouthillier : 25 % + one file from R4E (Tracer.java) which was done by Sebastien Dubois Dashboard.feature: Jacques Bouthillier : 100% Dashboard.ui: Jacques Bouthillier : 100%
(In reply to Jacques Bouthillier from comment #24) Thanks Jacques! I've ask EMO and they confirmed that Ericsson has a current Member Committer Agreement. I believe it's also reasonable to say that the work was done "under the supervision of the PMC", as demonstrated by the discussion on this bug report and your participations and demos in the Mylyn calls and responsiveness to feedback. Thus, figure #2 applies, which allows to accept the contribution without a CQ. http://www.eclipse.org/legal/EclipseLegalProcessPoster.pdf Let's continue with the technical discussion on the review.
(In reply to comment #24) > Jacques Bouthillier : 25 % + one file from R4E (Tracer.java) which I'm wondering if we could easily remove Tracer.java. It provides a logging API that could easily replaced with a standard logger (e.g. SLF4J).
I reviewed the comments, the only one I did not check yet is the Tracer.java and used the orbit slf4j. May be we can extend Tracer to implement the SLF4J. I refactor the Dashboard to be org.eclipse.mylyn.gerrit..... I put the versioning to 1.0.0 since it is not in incubator May be we need a CQ for "PowerMOCK for testing I kept the dashboard.site for the time being to test the functionality, later, it is just one line to add in org.eclipse.mylyn.reviews.site to include the Dashboard in there I also comment out the dashboard.ui.plugin to prevent the Diffy icon to show on the Eclipse toolbar for now. Code on Gerrit on the following link: https://git.eclipse.org/r/#/c/20073/
Steffen, When I pushed the code to Gerrit, it seems that there is a failure in the test. It looks like we execute on Helios (Eclipse 3.6) and egit for that eclipse version should be 2.1 or earlier, so egit 1.3 is fine. The test always fails, When I run with indigo (eclipse 3.7), the same test passes. Question, is Helios supposed to be supported or the minimum eclipse should be indigo with Mylyn Review?
Steffen, I was looking to execute the Junit test and maven compilation. We are using powermock 1.3.8 and it associated classes like javaassist-3.12 and objenesis 1.2, which are all packaged in the powermock 1.3.8.zip so we need to fill a CQ . Who should fill a CQ? Second, I will look at the Debugging functionality to use slf4J, but for now, if you do not activate the traces, nothing will ever show on the console. Jacques
(In reply to comment #29) > Steffen, > I was looking to execute the Junit test and maven compilation. We are using > powermock 1.3.8 and it associated classes like javaassist-3.12 and objenesis > 1.2, which are all packaged in the powermock 1.3.8.zip so we need to fill a CQ > . Who should fill a CQ? Before we go ahead and invoke the IP review process put this in Orbit etc. can you summarize why these libraries are needed? Would it be feasible to change the code to not require these dependencies? Mockito is already used in the Gerrit connector and I'd rather not introduce a second mocking framework.
The unit code was done a few months ago. We used powermock since we have static variables, static methods and powermock, which is and extension of mockito, seem the best unit test available for that purpose. What are the limitations of Mockito - Cannot mock final classes - Cannot mock static methods - Cannot mock final methods - their real behavior is executed without any exception. Mockito cannot warn you about mocking final methods so be vigilant. - Cannot mock equals(), hashCode(). Firstly, you should not mock those methods. Secondly, Mockito defines and depends upon a specific implementation of these methods. Redefining them might break Mockito. Jacques
I can see the value for testing legacy code but I wonder if the overall design could simply be changed to support dependency injection rather than mocking static and final methods. In any case, we should split that discussion from merging the dashboard. Can you remove the tests that require powermock for now? We can consider them in a separate change otherwise this might get blocked for a long time.
(In reply to comment #28) > Steffen, > When I pushed the code to Gerrit, it seems that there is a failure in the test. > It looks like we execute on Helios (Eclipse 3.6) and egit for that eclipse > version should be 2.1 or earlier, so egit 1.3 is fine. The test always fails, Mylyn uses Eclipse 3.6 as the base target. I believe Mylyn Reviews / Gerrit are using the standard base. We could probably change that to 3.7 if there is a need. > When I run with indigo (eclipse 3.7), the same test passes. Is that resolved now? > Question, is Helios supposed to be supported or the minimum eclipse should be > indigo with Mylyn Review? If feasible it should be Helios.
(In reply to comment #33) > (In reply to comment #28) > > Steffen, > > When I pushed the code to Gerrit, it seems that there is a failure in the > test. > > It looks like we execute on Helios (Eclipse 3.6) and egit for that eclipse > > version should be 2.1 or earlier, so egit 1.3 is fine. The test always fails, > > Mylyn uses Eclipse 3.6 as the base target. I believe Mylyn Reviews / Gerrit are > using the standard base. We could probably change that to 3.7 if there is a > need. > > > When I run with indigo (eclipse 3.7), the same test passes. > > Is that resolved now? > > > Question, is Helios supposed to be supported or the minimum eclipse should be > > indigo with Mylyn Review? > > If feasible it should be Helios. After adjusting the pom.xml, we can compile and run all test case on Helios, this one is solved
(In reply to comment #32) > I can see the value for testing legacy code but I wonder if the overall design > could simply be changed to support dependency injection rather than mocking > static and final methods. In any case, we should split that discussion from > merging the dashboard. Can you remove the tests that require powermock for now? > We can consider them in a separate change otherwise this might get blocked for a > long time. I removed the package for the Dashboard test cases since they all use Powermock, so no more Powermock in the current code review.
Push code to Gerrit, just finished compiling and two tests errors showed which are not in the Dashboard: Errors: testAccount(org.eclipse.mylyn.internal.gerrit.core.remote.GerritReviewRemoteFactoryTest): Wrong # responses: 0, updated: 0 Expected: is <1> but: was <0> testGetPatchSetPublishDetail(org.eclipse.mylyn.internal.gerrit.core.remote.GerritReviewRemoteFactoryTest): Wrong # responses: 0, updated: 0 Expected: is <1> but: was <0> 2 out of 178 tests failed
Hi Steffen, I am looking at slf4j for the tracing within the Dashboard. We can use different library (slf4j-simple-1.7.5, slf4j-jdk14-1.7.5, and the default slf4j-nop-1.7.5,jar). To use it, I need to add the library in the plugin runtime classpath. I don't like the idea to deliver the plugin with the library build in because the tracing will always be there. I need to use the slf4j-api-1.7.5.jar to allow the compilation, but how can I use the extra jar file (ex: slf4j-simple-1.7.5.jar) outside my plugin? I mean where do I store the jar file so I can just put the jar file somewhere, start eclipse and the logging will kick in. Is there any other things to do to activate the logging mechanism ? I noticed we can set the debug level using "-Dorg.slf4j.simpleLogger.defaultLogLevel=DEBUG" on the command line.
You would usually add a logging backend for SLF4J to your runtime configuration. I have used logback and log4j successfully. The logging is then configured for the respective backend. You could add org.slf4j.impl.log4j12 for example and then use a log4j.xml. I haven't tried the simple logger but that might be the simplest way to enable logging.
Some high level notes from a quick review: * All bundles need to have the standard about.html. The build.properties need to be updated accordingly to include the about.html for "src" and "bin". * To simplify version management I would consider making all versions 2.2.0 to they are aligned with the Gerrit connector and reviews. * In the UI I noticed that buttons display an error if no repository has been selected: "Use Button...", "You need to define...". Would it possible to popup the repository selection instead? That would be more intuitive. * The "Add Gerrit Repository" button only displays an error. It should be removed until it's implemented. * The Gerrit icon is used in two places (the view and toolbar). I would suggest replacing the toolbar icon with the repository icon (see Builds view for an example). * The "Task Repositories..." should not have the three dots and should be renamed to "Show Task Repositories" or "Show Task Repositories View" to make it more clear what the button does. * The "Current Query" label and text are redundant with the input box that also shows the current query. I would remove the "Current Query" label and text to make the header more concise. * The context menu has an entry "Toggle ID selection" seems that should be named "Star Review"? * If I select My > Watched Changes and then press Search I get a "Bad Request" error. * It looks like there is a margin around the table. That should be removed and the table to should expand to the size of the view like similar views in Eclipse.
(In reply to comment #39) Hi Steffen, I fixed most of the review items you mentioned. What is missing is: > * The Gerrit icon is used in two places (the view and toolbar). I would suggest > replacing the toolbar icon with the repository icon (see Builds view for an > example). Do you have an icon in mind, I can just replace it with your icon if you give me the icon. Should I use the old gerrit icon ? Second: > * In the UI, I noticed that buttons display an error if no repository has been > selected: "Use Button...", "You need to define...". Would it possible to popup > the repository selection instead? That would be more intuitive. I will look at it on Monday, but I am not sure if I can open a dialogue with the appropriate data selection before the dead line. To minimize the modification, I added in GerritTableView.java some constants which is the exact query performed . They should eventually be defined in GerritQuery.java file: MY_CHANGES_STRING = "owner:self OR reviewer:self"; MY_WATCHED_CHANGES_STRING = "is:watched status:open"; ALL_OPEN_CHANGES_STRING = "status:open"; I also pushed the latest code to Gerrit https://git.eclipse.org/r/#/c/20073/
- Fix the pom.xml to point to version 2.2.0 - Change the toolbar icon to show the same icon as the egit repository
(In reply to comment #40) > > * The Gerrit icon is used in two places (the view and toolbar). I would > suggest > > replacing the toolbar icon with the repository icon (see Builds view for an > > example). > Do you have an icon in mind, I can just replace it with your icon if you give me > the icon. Should I use the old gerrit icon ? The repository icon looks good. I would recommend using the blue one though rather than yellow one which stands out too much. Here a few more thing I noticed: * There is a grey border at the right of the table (see screenshot). * The column sizes and sorting is not persisted between workspace restarts. Also I can't hide/show columns. Take a look at org.eclipse.mylyn.commons.ui.TableViewerSupport which provides that functionality with a few lines of code. * It's difficult to see the difference between the repository label and "Total reviews" label. I think that needs a bit more spacing. * If the table is wider than the view the Search combo and buttons are not visible. It would be better to have a horizontal scroll bar on the table and ensure the button is always visible. * It's not clear to me what the value of the "Documentation" drop down is. I'd rather remove that and have a context sensitive help for the search. * Pressing the "My" button runs the "Watches Changes" query. I would recomment moving the "Watched Changes" item to the top of the list to make that move obvious. * Pressing enter in the search combo does not have any effect. I would expect that to execute the query. * I would consider removing the border that surrounds the repository label and search combo. It adds clutter to the UI and takes up quite a bit of space (at least on Mac).
Created attachment 240036 [details] extra margin
Hi Steffen, Concerning the comment on the patch set 12, The about.html are part of each package since patch 11 The warning I see in the hudson build are related to plugin.properties not found, they do not refer to the gerrit.dashboard, but to the following packages: org.eclipse.mylyn.gerrit.core/plugin.properties not found org.eclipse.mylyn.gerrit.core.tests/plugin.properties not found org.eclipse.mylyn.reviews.ui/plugin.properties not found org.eclipse.mylyn.gerrit.ui/plugin.properties not found org.eclipse.mylyn.versions.tasks.mapper.generic.tests/plugin.properties not found org.eclipse.mylyn.gerrit.core/plugin.properties not found ...
(In reply to comment #42) > The repository icon looks good. I would recommend using the blue one though > rather than yellow one which stands out too much. OK > > Here a few more thing I noticed: > > * There is a grey border at the right of the table (see screenshot). Need investigation why? Looks at the exact space reserved by the scroll ? > * The column sizes and sorting is not persisted between workspace restarts. Also > I can't hide/show columns. Take a look at > org.eclipse.mylyn.commons.ui.TableViewerSupport which provides that > functionality with a few lines of code. I will have a look > * It's difficult to see the difference between the repository label and "Total > reviews" label. I think that needs a bit more spacing. OK > * If the table is wider than the view the Search combo and buttons are not > visible. It would be better to have a horizontal scroll bar on the table and > ensure the button is always visible. I agree, but happen if the end-user makes it window width very narrow, there is a minimal size for the portion above the table. Will not fixed it right now > * It's not clear to me what the value of the "Documentation" drop down is. I'd > rather remove that and have a context sensitive help for the search. Context sensitive in our backlog, but require more time to implement, caanot do it for the first release. Also, the Documentation is an option available from the Gerrit WEB interface, this is the reason we implemented it so the end-user can query the Gerrit Database > * Pressing the "My" button runs the "Watches Changes" query. I would recomment > moving the "Watched Changes" item to the top of the list to make that move > obvious. OK > * Pressing enter in the search combo does not have any effect. I would expect > that to execute the query. Will see what I can do shortly > * I would consider removing the border that surrounds the repository label and > search combo. It adds clutter to the UI and takes up quite a bit of space (at > least on Mac). It looks like a focus border which I don't see on window, I Remove the option SWT.BORDER when creating the widget hoping this solved this issue. Concerning previous comments what is missing now is the Task repository selection. When there is no pre-selected Gerrit definition, the popup shows. If there is only one Gerrit definition , then it is selected automatically. If there is more than one Gerrit server, I will look to open a Dialog with radio buttons selection, but is not done yet
I look at : * The column sizes and sorting is not persisted between workspace restarts. Also > I can't hide/show columns. Take a look at > org.eclipse.mylyn.commons.ui.TableViewerSupport which provides that > functionality with a few lines of code. The way the table is currently build is more than just a few lines. Also, with the modification we have in mind to have the LABEL columns being define dynamically, we will look at this issue at that time if it sounds ok wih you. I fixed the enter key in the search text to activate the command. I set a minimal with of the text before issuing the command since the shortest command I found is : "is:open" I push another build to gerrit with the latest update. https://git.eclipse.org/r/#/c/20073/12
Code rebase Initial contribution pushed to the Mylyn master for version 3.11
Thanks for merging the change. Jacques, please make sure that all packages in the new bundles are exported as x-internal otherwise we'll end up with API errors once we update the API baseline.
Here is a review to add the x-internal/x-friends to the exported packages: https://git.eclipse.org/r/22187
Thanks Gunnar! I merged the review.
OK, so I'm marking this fixed now. The dashboard has been merged. We'll open new requests for improvements. Jacques, please make sure to open new bugs for things in your backlog / on your todo list.
Ok I will do Thanks
(In reply to Steffen Pingel from comment #42) > * I would consider removing the border that surrounds the repository label > and search combo. It adds clutter to the UI and takes up quite a bit of > space (at least on Mac). Steffen, do you mean the edge surrounding the Group? At first, I thought you meant the blue line surrounding the text field but I don't think that can be disabled since this is default behavior on Mac.
(In reply to Marc-Andre Laperle from comment #53) > Steffen, do you mean the edge surrounding the Group? At first, I thought you > meant the blue line surrounding the text field but I don't think that can be > disabled since this is default behavior on Mac. Right. It's the Mac focus indicator.
(In reply to comment #53) > Steffen, do you mean the edge surrounding the Group? Yes. I'd remove the group and use a simple composite. Groups are useful in dialogs with many controls to create visual separation. In the case of the dashboard I would argue that the table and view borders are already sufficient to create visual separation.
