Bug 370050

Comment 1 John Arthorne 2012-01-30 10:09:46 EST

If you uncomment those lines in OpenResourceDialog.js, the star icon will appear with a grey border around it, which looks a bit ugly. From debugging with Chrome dev tools I couldn't find any CSS indicating a border of any kind around the image or surrounding elements, so I can't figure out where this border is coming from. Probably something obvious I'm overlooking here.

Comment 2 Susan McCourt 2012-01-31 22:22:52 EST

fixed.  I knew this seemed familiar, I had a similar problem with icons in the navigators.  Finally remembered.  In Chrome, if you create an image node without a valid src, you get the ghost border.  So I assigned none.png to the image.

Comment 3 John Arthorne 2012-02-02 17:00:18 EST

I had to overwrite this change because Andrew and Kris did a major refactoring of this code. I am reopening this so we can find a way to address it in the new code. This is no longer so easy because the rendering logic has been made generic and the same code is used for both favorites and other search results.  For reference here is Susan's working code that needs adding back:

var image = new Image();					
dojo.addClass(image, "commandSprite");					
dojo.addClass(image, "core-sprite-makeFavorite");					
dojo.addClass(image, "commandImage");					
// without an image, chrome will draw a border  (?)					
image.src = require.toUrl("images/none.png");					
col.appendChild(image);

Comment 4 John Arthorne 2012-02-02 17:04:31 EST

My current thinking is that the way to do this is to add the image or image class as part of the "resource" objects passed to the render function. This could be used to associate other kinds of images with files later (for example folders could have a folder icon, different kinds of files could have appropriate icons, etc).

Comment 5 Andrew Eisenberg 2012-02-02 17:23:10 EST

One potential solution is to pass in a decorator function to mkeRendererFunction in searchRenderer.js. The decorator would get passed a table row and be able to manipulate it as required (in this case, it wouldadd the images).  And of course, if no decorator is passed in, then there would be no change in behavior or presentation.

Comment 6 Andrew Eisenberg 2012-02-02 17:26:13 EST

(In reply to comment #4)
> My current thinking is that the way to do this is to add the image or image
> class as part of the "resource" objects passed to the render function. This
> could be used to associate other kinds of images with files later (for example
> folders could have a folder icon, different kinds of files could have
> appropriate icons, etc).

With this solution, you would be hard-coding the images with the resource kind, which is a good way of standardizing things across the IDE.  The suggestion in my previous comment leaves the decorating power in the hands of the caller of the searchRenderer.  I am not sure which is more appropriate.

Comment 7 John Arthorne 2012-02-03 11:38:59 EST

(In reply to comment #6)
> With this solution, you would be hard-coding the images with the resource kind,
> which is a good way of standardizing things across the IDE.  The suggestion in
> my previous comment leaves the decorating power in the hands of the caller of
> the searchRenderer.  I am not sure which is more appropriate.

I like your decorator function idea.. sounds a bit like a JFace label decorator. We could still use that to standardize by having a "file label decorator" that we consistently use. Since the JSON object coming back from the server won't be able to make a decision about what icon to use, this information needs to be added at some point after the fact by the client anyway.

Comment 8 Susan McCourt 2012-02-03 11:41:09 EST

(In reply to comment #6)
> (In reply to comment #4)
> > My current thinking is that the way to do this is to add the image or image
> > class as part of the "resource" objects passed to the render function. This
> > could be used to associate other kinds of images with files later (for example
> > folders could have a folder icon, different kinds of files could have
> > appropriate icons, etc).
> 
> With this solution, you would be hard-coding the images with the resource kind,
> which is a good way of standardizing things across the IDE.  The suggestion in
> my previous comment leaves the decorating power in the hands of the caller of
> the searchRenderer.  I am not sure which is more appropriate.

I haven't looked at the new renderer code, but I can comment on what explorer does in the explorer renderers.  We let the caller specify the image class, for example
org.eclipse.orion.client.core/web/orion/explorer.js line 397.

So where and how the decorator is positioned within the renderer is somewhat hardcoded and extenders can decide which icons are used.  We also have places where a renderer "just decides" in line with a hard coded decorator function 

org.eclipse.orion.client.coreweb/orion/explorer-table.js line 89

I wouldn't want to see decorator objects and lots of infrastructure, but a simple function might be appropriate.  Perhaps we could avoid more "copied code syndrome" in the future by having some standard static decorator functions in util.js that handle the common cases. 

For example I would imagine that explorer "add image to link" could be generalized a bit.

Comment 9 Susan McCourt 2012-02-03 11:43:18 EST

mid-air collided with John.  Changing owner back to Andrew.

Comment 10 Andrew Eisenberg 2012-02-03 17:58:48 EST

I have a fix for this that implements what I suggested.  However, I am not happy with the way it looks for 2 reasons:

1. With the images displaying, each line of the favorites takes up more room and so the overall dialog is larger without conveying any extra information.
2. The presence of the star next to the file name, implies to me that I should be able to click it (since that's what stars elsewhere imply).  

Suggestion: perhaps having only one star at the top left of the favorites list would be sufficient.

Once we're all happy with the look of this, I can refactor the decorator into util.js and submit a new patch.  Is there any other similar code around that should also be pulled out into util.js?

Here's the branch:
https://github.com/kdvolder/orion.client/tree/Bug370050

Here's the commit:
https://github.com/kdvolder/orion.client/commit/8ada97d83ad483f886e481f58683b3f4f0674ffb

Comment 11 Susan McCourt 2012-02-07 13:12:22 EST

(In reply to comment #10)
> I have a fix for this that implements what I suggested.  However, I am not
> happy with the way it looks for 2 reasons:
> 
> 1. With the images displaying, each line of the favorites takes up more room
> and so the overall dialog is larger without conveying any extra information.
> 2. The presence of the star next to the file name, implies to me that I should
> be able to click it (since that's what stars elsewhere imply).  
> 
> Suggestion: perhaps having only one star at the top left of the favorites list
> would be sufficient.
> 
> Once we're all happy with the look of this, I can refactor the decorator into
> util.js and submit a new patch.  Is there any other similar code around that
> should also be pulled out into util.js?
> 
> Here's the branch:
> https://github.com/kdvolder/orion.client/tree/Bug370050
> 
> Here's the commit:
> https://github.com/kdvolder/orion.client/commit/8ada97d83ad483f886e481f58683b3f4f0674ffb

for #1, it is conveying extra information in my opinion...it's telling me why some "random list of files" is at the top of that dialog.  I also thought about a heading (comment 0) but I was actually worried more about it seeming too crowded than taking up too much space.  When I first implemented it, i was skeptical, but I thought it looked okay.  I felt like a heading was a new metaphor I had to visually parse whereas I see the star and I know where those things came from.  

with respect to #2, I don't have a problem with that so much because we use "star on the left" to mark a favorite in other places.  (like on the menu of move/copy target choices).  I think people can figure this out.

So I'd be fine with having something similar to the original fix and then people can complain if it's truly eating up too much space or confusing them.  I think I'd want proof (via bugs) that the star wasn't working before introducing a new visual pattern.

Comment 12 Andrew Eisenberg 2012-02-07 13:18:07 EST

Fair enough.  I'm ok with this.

The commit is still available.  Let me know if you'd prefer me to pull the code out to util.js or somewhere else.

Comment 13 John Arthorne 2012-02-07 17:12:52 EST

Merged and pushed:

http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=c80f4019582ce7803c39be788e831f921b360956

A couple of changes:

- I switched to showing stars on the left so it lines up nicely. I had to set the vertical alignment on the cell for it to line up properly

- I noticed you changed some jsdoc of the form "@param [arg]" to "@param arg (optional)". The square bracket notation for optional arguments is specified for jsdoc. The jsdoc generator will describe this as an optional argument so we should stick with its notation. It might also have tooling value in the future to be able to infer optional arguments so following the convention is useful.