This looks like a very nice addition. The only thing I am not crazy about is persisting the ui config in the user service. I think that show starred/show all would better be handled by a new Gitblit settings cookie. What do you think of that?

As you may use different computers at the same time (like I do - most times I use two in parallel, sometimes three) I think it would be better to have the settings persisted on server side. Otherwise I have to select my favorites three times. Furthermore, developers tend to clear their caches from time to time and if they develop a web app, some of them might need to clear the cached cookies as well and they probably will feel bothered selecting which cookies to delete and which cookies to keep each time. From the perspective of usability for the gitblit user I think it's better to persist the settings on server side.

Other opinions?

I've reviewed this more thoroughly and here are my thoughts.

1. I like the idea of starring. I think the language should be "star" all the way down through the code to the user service persistence. "Selected" is ambiguous and can refer to anything. I think we can get by with a tooltip instead of showing star/unstar text along with the icon.
2. I would like to be able to star individual repositories, not just projects so I would amend the functionality to use regex such that starring a project sets a value like "myprojects/*".
3. I want to be able to star/unstar a repository from the project view (https://demo-gitblit.rhcloud.com/project/main) and from the repository page in the fork controls (https://demo-gitblit.rhcloud.com/summary/gitblit.git). From the repositories page I should be able to star/unstar projects - just like you have it.
4. Renaming a repository must also update starred repository settings for all users.
5. I do not want to rewrite the entire user service file when a user changes their UI view for showing starred/unstarred repositories & projects so I do not support persisting this UI preference in the user service. I would prefer either a cookie or perhaps a separate server-side user preference storage.
6. When we test for if the user is logged in to show/hide elements currently a null test is sufficient, but this needs to be extended to a test for anonymous too since at some point I want to always have a user object instead of relying on null.
7. When using the translations within the web UI we must use Wicket's built-in translation service. Wicket's service accounts for the user's browser preference. The Translation class only returns the translation for the server-side locale preference. I know this is confusing because the Translation class is so tempting to use, but that is meant for application code like the Manager. In the future I hope to have this project broken up into a few smaller projects where this is more clear but that is a separate task.

Hi James,

Alice and me just discussed your feedback. we would like to divide the changes to make into two packages. So, we concentrate on the core functionality first, then we merge back to the master, before we extend the UI again.

There is one question remaining regarding 7.: What should we use instead of the Translation class then? Can you give us an example or point us to a line in the existing code, which does it the right way?

We think that we can finish 1., 4., 5., and 6. (and probably 7. if we know how to do it correctly) until end of next week.
I think we should merge then first before we add repository specific selection (regex support) and more UI components to use the feature (2. and 3. of your feedback).
What do you think about this strategy?

That sounds fine.

I'm having a hardtime finding an example in my code. :( You can look at NavigationPanel to see the use of getString(key). But you should also notice that this method is used inside a repeater. Wicket complains obnoxiously if you call getString from the constructor of a panel. In your implementation you would have to do that or pass a Localizer instance from the owning page as a constructor argument.

One alternative is to specify the translation key in your markup using the wicket:message notation but to do that you would have to create two buttons and show/hide buttons. Another alternative would be a dropdown list:
http://twitter.github.com/bootstrap/components.html#buttonDropdowns

I would favor a dropdown here because there may be reasons to add more selections in the future; a toggle button does not scale. Gitblit currently has Bootstrap 2.0.4 (I think) so the docs may be out of sync.

One thing I try to keep tight control of is Wicket page sessions. When you use Wicket links or AJAX you create a serialized version of the page in a user page-session-store. For some pages this is absolute must (Edit Repository) for most, though, I try hard to avoid that and to keep pages session-less.

This is not the same as the servlet container session. A page with Wicket state will lead to link failures if Wicket thinks your page has expired. One way I work around that is by minimizing use of Wicket-generated links by passing explicit urls with parameters REST-ish or by storing important state in the user session. It may not be a problem here, but it is something to keep in mind.

A good example of this is the repositories page for an Admin or Owner and the delete repo links. Those will fail after a session timeout because they use Wicket urls, Javascript, and rely on the a page session store. In contrast, the edit links should always work because they are manually generated and point to REST-ish urls. I'm not in love with Wicket, but it is what we've got.

As for the regex, I don't think you need anything different UI-wise to support regex. It should be straight-forward and internal.
to star a project = user.addStarredThingy("myproject/*")
user.isStarredThingy(string) should do the regex matching like user.hasRepositoryPermission

I also now notice this line:
String rootPath = StringUtils.getRootPath(model.name)

You shouldn't need that. Check out RepositoryModel.projectPath.

Hi Alice & Sarah,

I have implemented a slightly different starring feature - which at the moment does not touch the repositories page, but likely should. Thanks for contributing your ideas and participating in a discussion with me, even though I ultimately decided to implement this slightly differently. Good luck with your scm-manager transition!

-J