Pull Request: Add a friendly name to package sources (issue 98)

Oct 15, 2010 at 7:23 PM

I figured I'd try out a relatively small but popular request for my first contribution.

I've updated the Tools | Options page to include a Name field for adding package sources. I converted the original ListBox to a ListView control to support columns for Name and Source. I also added a green checkmark icon to indicate the default package source.

I then updated the Package Manager Console to show the friendly name in the drop down instead of the source. 

I've uploaded some screenshots here: http://www.systemex.net/NuPack/issue98/

Let me know what you think.


Oct 15, 2010 at 7:27 PM

The screenshot looks very nice. Before we can review your changeset, can you create a review request on the review board (http://nupack.codeplex.com/wikipage?title=Code Reviews)?


Oct 15, 2010 at 7:32 PM

Fixing the link: http://nupack.codeplex.com/wikipage?title=Code%20Reviews

Oct 15, 2010 at 7:32 PM

Also, should we visually indicate that the name is optional somehow? Maybe as simple as (optional) to the right of the text box (or follow whatever Windows UI guidelines there may be on this).

Oct 15, 2010 at 7:38 PM
Looks pretty good to me. Be ready. The code convention nazis get me every time. ;-)

Oct 15, 2010 at 7:43 PM

@Haacked, kind of surprised that you consider the Name to be optional. Currently, new package sources use the name "New".

My update has it as required (can't Add without filling in both Name and Source). However, I could make it optional and use the "Source" as the name.

I'll be submitting a review request shortly.

Oct 15, 2010 at 7:44 PM

Another thought. If you were in the package console, wouldn’t you want to at least see the domain name for the current project without having to go to the settings dialog? For example, maybe the dropdown looks like:

[Official Package Source (nupack.example.org)]

[My Package Source (example.com)]

Also, if you don’t specify the package name, I assume the dropdown will show the URL as a fallback, right?

Oct 15, 2010 at 7:45 PM

Oh, well let’s discuss that. Should it be required? That certainly makes it easier. If so, what does the validation look like if you click OK and leave it blank?

Oct 15, 2010 at 7:50 PM

I agree that the console's dropdown should show the domain name as well.

Oct 15, 2010 at 7:53 PM

We need to do a bit of an overhaul on the UI for this (putting my graphic designer hat on). We've lost the path to completion on the form and now there's multple fields and multple buttons to click to accomplish multiple tasks. At some point someone will add another field and it'll just be even messier.

I like it, just need to put in maybe a default (like how new projects are ProjectX when you add them) so maybe "Package Source n" for the friendly name.

IMHO both name and source should be required (and if you default name then users only have to fill out one field)

Oct 15, 2010 at 7:54 PM
Yeah - makes sense. Should it truncate at some point though? Or is it good to make it really long and just let the box cut it off. How long are we allowing people to name their items now? What happens if I call one:

"a;lsdjfa;lksjf laksjdflk ajsdlfkjasl dfjalksj lkasjd flkasjd flkajs dlkfjasl kfjaslkjfalksj flkasjdflkasjdflkasjdfklasjdflkajsdl kfjaslkfjaslkjfalksjfa;lsjfa;slkfjaslkdjflaksjdf lkajsdf lkjasdl;fk jasl;fj alksjdf lkasjdflk;asjdflk;asjdflk;ahtgaweifja;kbnkalfaskljefawel;idjflkajsf"

Is that valid? Should it be? ;)
"Be passionate in all you do"

Oct 15, 2010 at 7:55 PM

You can't click Add unless both Name and Source are populated. If you entered Source and clicked OK, the dialog would close and nothing would be added. That's the same behavior in CTP1. User must explicitly click Add to add a new package source.

As for adding the domain to the drop down, what about local paths? I suppose we can simply append the source to the path.

I'll add a screenshot for discussion.

Oct 15, 2010 at 7:58 PM

I took a brief look at the changeset and saw that several places do not follow our coding convention. Could you make a sweep through it before submitting review request?

Oct 15, 2010 at 8:01 PM

@dotnetjunky: Perhaps you can point me in the right direction? I've updated VS/Resharper to place opening brace on first line. Not sure what other coding conventions you're using that I'm not following.


Oct 15, 2010 at 8:08 PM
"Be passionate in all you do"


Oct 15, 2010 at 8:09 PM

I added a screenshot for displaying "Name (Source)" in drop-down list. http://www.systemex.net/NuPack/issue98/ (scroll to bottom)

Let me know what you think.


Oct 15, 2010 at 8:10 PM
kiliman wrote:

@dotnetjunky: Perhaps you can point me in the right direction? I've updated VS/Resharper to place opening brace on first line. Not sure what other coding conventions you're using that I'm not following.


 For example, this line:

 if (_onItemBound != null) _onItemBound(o, item);

We always wrap the body of the 'if' statement in a block { }. There are several other places. You can follow the guidelines that ferventcoder just posted.

Oct 15, 2010 at 8:11 PM

Rob, I saw the guidelines, however, I don't see where I'm not following them. That's why I'm asking for a hint.

Oct 15, 2010 at 8:12 PM

Hmmm. Not sure this will work. It’s great for a short url or path but when you get long ones (say 100 characters) then screen real estate becomes a real problem.

I know it’s just a drop down and maybe not a lot of control (I haven’t looked but it’s WPF right?) so I see alternatives as:

1. Just display the name

2. Display the name but put the url in a tooltip

Oct 15, 2010 at 8:15 PM

The dropdown is not WPF. It's a native VS combox box control.

Oct 15, 2010 at 8:15 PM

@dotnetjunky: Ok, thanks. You might want to add that to your guidelines then, because it doesn't explicitly state that you want braces around single line statements.

I'll update and repush. Now, do I need to issue a new pull request? First time doing this though CodePlex, so not sure what the proper procedures are. 


Oct 15, 2010 at 8:17 PM

You could still do the package source as tooltip though even with a native combo.

Oct 15, 2010 at 8:18 PM

I don't think we need a pull request until we're done in review board. The review board walk-through (linked above) includes information on how to "update" the review as you iterate.

Oct 15, 2010 at 8:21 PM

Package source URL as a tooltip makes sense.

Oct 15, 2010 at 8:58 PM

@Haacked, not sure if that is possible. I'm having trouble finding where these drop-downs are created. I see in PowerConsoleToolWindows.cs where we are providing the list of sources, but I don't see how to hook into the creation of items to set the tool tip.

So unless somebody has any suggestions, I don't think this should impact acceptance of my patch.

Oct 15, 2010 at 9:11 PM

Unlike WPF, we don't control the creation of items in the combobox. We return the list of strings and the control shows the items itself. It may not even be possible to provide tooltips for the items.

I'd say go ahead and send out the review without the tooltips. I will consider converting the entire toolbar into Wpf at some point.

Oct 15, 2010 at 9:14 PM

Extension method on the list item or something? Yeah, I say go ahead without the tooltips we can sort it out later.

Oct 15, 2010 at 9:15 PM

@dotnetjunky: I'm having problems updating my review. I made the changes you requested for following guidelines. Committed to local repo. Pulled in mainline repo and merged into local. Committed the merge then pushed to my fork.

When I run hg postreview -e 64 (my review id), I get the error: abort: The file was not found in the repository (207)

Not sure what I need to do to update the review.


Oct 15, 2010 at 9:20 PM

Agree about not holding up the review of this request. We can file new bugs for these minor UI changes as we think of them.

Oct 15, 2010 at 9:22 PM

I saw your request sent out. Do you still have trouble?

Oct 15, 2010 at 9:31 PM

Somehow, it created a new review. It didn't update the existing one. I'm still not able to add the new diffs to my existing review.

Should I just delete the review and start over? Sorry if I'm making a mess, but this is the first time using these particular tools.

Oct 15, 2010 at 10:45 PM

We're all getting accustomed to these tools. No biggie. We've been trying to rebase instead of merge (we have a nice straight line for our graph), but if it becomes too much of a hassle then you can go ahead and merge. Are you in sync with main?

Oct 15, 2010 at 10:55 PM

Yes, I have the latest commit from dotnetjunky (af3c89bf93ef) in my fork. If I can back out the merge, I'll try rebasing.


Oct 15, 2010 at 11:01 PM

I think I'm going to just delete my fork and try again. Chalk it up to learning experience.

Oct 15, 2010 at 11:31 PM

Ok, I started from scratch. Deleted the original fork and created a new fork. Applied my changes, committed, and pushed.

I discarded my original review and submitted a new one.

Hopefully this will go smoother. Sorry for any inconvenience.

Oct 15, 2010 at 11:47 PM

No problem, I think it's good to go through this process so we can make it smoother later. I'm working on guidelines for contributor code reviews.

Oct 16, 2010 at 2:15 AM

Btw, do you put a Max Length constraint on the Name textbox? I think we should put a length limit on it, like 50 characters or so.

Oct 18, 2010 at 8:13 PM

Ok, I've made all the changes that were requested in the review. Hope you guys get a chance to look at it so it can be added to NuPack.