Bugzilla – Full Text Bug Listing
| Summary: | [ContentAssist] Allow more fancier proposals (similar to the ones in the Eclipse IDE) | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Thomas Schindl <tom.schindl> | ||||||
| Component: | Editor | Assignee: | Project Inbox <orion.editor-inbox> | ||||||
| Status: | RESOLVED WONTFIX | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | andrew.eisenberg, Holger.Schill, john.arthorne, mamacdon | ||||||
| Version: | unspecified | Flags: | mamacdon:
review+
|
||||||
| Target Milestone: | --- | ||||||||
| Hardware: | PC | ||||||||
| OS: | Mac OS X | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Thomas Schindl
I've implemented the attributed string now as well - from a security point of view this is the safest way - I think. See https://github.com/tomsontom/orion.client/commit/2fe7a13bd8c0744222c26c879f0fc006ac1bb55a Very nice. I played around with this and got some good looking proposals. Here are some comments on the code:
* squash to single commit when it is all ready for including. Add this commit to a new branch so I can easily apply to my clone.
* why description.icon.src? why not description.icon?
* description should allow for non-styled: eg-
description = {
icon: 'http://foo'
value : 'my description'
}
instead of:
description = {
icon: 'http://foo'
segments : [ {value : 'my description'} ]
}
This covers the case where the only styling is the icon itself
* use if(segment.style.fontName) if(segment.style.fontName != null) and similar for the other null checks
* the description object seems a bit complex to me. When I was playing around with getting this displaying something interesting, it took me a long time to set up the object properly. I gave two suggestions for simplifying above. And I thought about removing the 'style' segment and just having segment.fontName directly, but I kind of like being able to -pre-define styles beforehand and add them to proposals. So maybe things are fine after this.
* I have no comments on styleClass. I am not too fussed about it, but I do want someone else from the orion team to comment on it.
Adding Mark as a reviewer for styleClass. (In reply to comment #2) > Very nice. I played around with this and got some good looking proposals. > Here are some comments on the code: > > * squash to single commit when it is all ready for including. Add this > commit to a new branch so I can easily apply to my clone. > > * why description.icon.src? why not description.icon? > I first had only icon but then thought it might make sense for the future to allow e.g. setting the alignment, padding, ... of the image so I opted for an icon-object and the src-attribute > * description should allow for non-styled: eg- > description = { > icon: 'http://foo' > value : 'my description' > } > > instead of: > > description = { > icon: 'http://foo' > segments : [ {value : 'my description'} ] > } > Ok that makes sense, I'll add this > This covers the case where the only styling is the icon itself > > * use if(segment.style.fontName) if(segment.style.fontName != null) and > similar for the other null checks > ok I think a reviewer has to be cc'ed to get mails. Here's the diff to master: https://github.com/tomsontom/orion.client/compare/master but this one also holds the changes described in bug #392073 > > * why description.icon.src? why not description.icon?
> I first had only icon but then thought it might make sense for the future to
> allow e.g. setting the alignment, padding, ... of the image so I opted for
> an icon-object and the src-attribute
One of the nice things about JavaScript is that it is very easy to migrate the api and handle multiple shapes of the icon property.
if (typeof description.icon === 'string') {
// the simple shape
} else {
// assume description.icon.src
}
(In reply to comment #7) > One of the nice things about JavaScript is that it is very easy to migrate > the api and handle multiple shapes of the icon property. It's nice that JavaScript allows it, but in the end it doesn't make for a clean API. Documenting a field as possibly a string, and possibly a rich object, is messy and make it harder on the tooling and harder on the receiving end. So if we can reasonably expect wanting to add more properties to an image in the future I like the object approach.
> > * description should allow for non-styled: eg-
> > description = {
> > icon: 'http://foo'
> > value : 'my description'
> > }
> >
> > instead of:
> >
> > description = {
> > icon: 'http://foo'
> > segments : [ {value : 'my description'} ]
> > }
> >
>
> Ok that makes sense, I'll add this
>
Rethinking this does it really make sense? If you want attributedString then use attributed string instead of handling one more special case. What would happen with description { value : "my desc", segments : [ { value : 'bal bla' } ] }.
Who would win, would both be printed?
(In reply to comment #8) > It's nice that JavaScript allows it, but in the end it doesn't make for a > clean API. Documenting a field as possibly a string, and possibly a rich > object, is messy and make it harder on the tooling and harder on the > receiving end. So if we can reasonably expect wanting to add more properties > to an image in the future I like the object approach. (In reply to comment #9) > Rethinking this does it really make sense? If you want attributedString then > use attributed string instead of handling one more special case. What would > happen with description { value : "my desc", segments : [ { value : 'bal > bla' } ] }. > Both of these comments are making the same point that the API should be both precise and verbose. This is the way things typically are (and should be) done in the Java world. But, as I am getting more experience with JavaScript, I see how things are handled differently. API methods are typically overloaded to take multiple kinds of arguments. Take jQuery as an example: $(window.document.body) has the same value as $("body_id') assuming the document looks like this: <html><body id='body_id'></body></html>. There are also many forms of the require and define methods for AMD-style modules. We also see it in the content assist proposal API itself, where the proposal can be a simple string or a complex object. Documenting this kind of flexible API is also straight forward (even though it is long and ugly) using Closure-compiler style jsdoc comments: /** @type {{value:String,icon:String} | {segments:{ Array.<{value:String,style: {bold:Boolean, italic:Boolean, color:String}}>}, icon:String}} Some explanation here */ var description; (This probably won't be formatted particularly well once the comment is accepted.) Orion already has some understanding of these kinds of jsdoc (except doesn't yet recognize parameterized arrays or union types) for inferencing and content assist. Here is a summary of my points: 1. This kind of flexible API seems to be common in the JS world. 2. This is not a big problem for tooling, even though Orion inferencing isn't quite able to handle it yet 3. I would like to keep the simple things simple for plugin developers, while not preventing the more complex things. I do understand that providing a flexible API can be a slippery slope. How flexible should it be and where should we stop implementing variants of API methods? I think that this is just a situation where we will need to use good judgement and apply our experience. And to answer this particular question: > Who would win, would both be printed? The JS way would be to tolerate this and do something "reasonable" and documenting it in the API. However, I would prefer throwing an exception and making it explicit that the API has been broken (maybe this is my Java background leaking through). Happy to hear your thoughts on this. Just to be clear: I am not claiming to be a JS expert. I just want to ensure that the most idiomatically appropriate API is created (and trying to learn a few things in the process). So I am perfectly willing to be called on this if you don't agree. I agree it is different in the JS world. The difference is that the consequence of getting it wrong in Java is much worse because you are more limited in how API can be evolved. So a good API in Java errs on the side of more general, for example taking a class as argument that can be augmented in the future with more members. This does have the side-effect of making API that is heavy-weight and more unwieldy to use. In JS we can afford to be more concise because we have more options to get out of trouble later. Before working in Java I used Smalltalk and worked with code that took advantage of the weak typing to do similar tricks. I remember it came in handy, but would also get us into trouble. More permutations of argument or return types makes a function harder to test. An implementer of a function may not properly handle one of the possible argument types, or a caller may not properly handle one of the possible return types. This might not be noticed until much later because all of your own code might only use one of the two available types. In the end I agree with most of what you said. Especially the part about using our judgement and experience to decide in a particular case. In Java I would have said, go with an object if it's *possible* we would need a richer argument later. In JS I would say, go with an object if it's *likely* we will need a richer argument later. But that's just my $0.02 based on current limited experience and I'm happy with whatever you decide in this case. My comments (not just on styleClass): 1.) There's a merge problem with commit 6915ef1 : the file bundles/org.eclipse.orion.client.editor/web/orion/editor/AsyncStyler.js is missing. The editor page (edit.html) needs this file to load, so this has to be fixed. 2.) The "nobr" element is unnecessary -- we can get the same effect with CSS. I'd recommend adding a generic "proposal" class to every contentDiv, and then add a rule like this to org.eclipse.orion.client.core/web/css/ide.css (where the other content-assist related styling lives). > .contentassist .proposal { > white-space: nowrap; > } You can probably also remove `contentDiv`, and just make the segments as direct children of the proposal `div`. 3.) "fontName" is not a valid style property AFAIK -- it should be "fontFamily". 4.) The use of image URLs concerns me somewhat, as each URL represents a potential HTTP request. We can mitigate this by encouraging the use of data-URIs in preference to image URLs, but then the proposals data structure could get quite large and contain a lot of redundant image data. 5.) Regarding "styleClass" within an "attributedString" proposal -- I'm not clear on what the purpose is. Is this to expose a CSS class name so external stylesheets can target it? Moreover, if we're going to add "attributedString", do we really need to keep our existing styleClasses ('proposal-emphasis', 'proposal-noemphasis', etc.) around? For API clarity, I'd prefer having 1 single way to style proposals, rather than 2 potentially-overlapping ways. With points #4 and #5 in mind, what do you folks think about making styleClasses extensible, and incorporating the kind of style information Tom's created for attributedStrings? Rather than making each proposal carry its style info, we could define the styleClasses separately (with a different service name, to avoid overloading orion.edit.contentAssist). Then a proposal would simply carry a styleClass field referencing the styleClass that provides its styling, and also carry the values for the segments. For example, a Java content assist plugin might define some styleClasses like this: > registerService('orion.core.contentassist.style', {}, { > styleClasses: [ > { > id: 'public-method', > icon: { src: 'data:image/png;base64,...' }, //image data goes here > segments: [ > { color: 'black', bold: true }, > { color: 'gray' } > ] > }, > { > id: 'private-method' > icon: { src: 'data:image/png;base64,...' }, > segments: [ > //whatever style is appropriate for private methods > ] > } > }) then an individual proposal would be assigned a styleClass by having this shape: > { > description: { > styleClass: 'public-method', > segments: [ > 'bla() : void', // value of first segment > ' - Test' // value of second segment > ] > } > } I've made the assumption that most clients will have a finite set of styleClasses (eg. public, private, protected) into which proposals are categorized, so arbitrarily styling an individual proposal would not be necessary (although I guess we could add it later if needed). Interested in hearing your thoughts. > With points #4 and #5 in mind, what do you folks think about making
> styleClasses extensible, and incorporating the kind of style information Tom's
> created for attributedStrings? Rather than making each proposal carry its style
> info, we could define the styleClasses separately (with a different service
> name, to avoid overloading orion.edit.contentAssist). Then a proposal would
> simply carry a styleClass field referencing the styleClass that provides its
> styling, and also carry the values for the segments.
I do like the idea of separating the style from the text. It would be easy for Orion to define a few generic styles that can be shared across many plugins. Then plugin devs would have less of a need to define their own custom styles.
Regarding image urls vs data-uris, is it not the case that there will only be one http request per unique url (no matter how many times that url is used)?
I would be fine deprecating the existing proposal.style that we have now in favor of using styleClass.
I would also be less concerned about having a flexible api for the styleClass if we went with this approach since fewer plugin devs would be creating their own styles. This would also significantly simplify what a styled proposal looks like, which I like.
I would expect that we would still support a simple string as a description if no style information is required.
(In reply to comment #14) > Regarding image urls vs data-uris, is it not the case that there will only be > one http request per unique url (no matter how many times that url is used)? I think that depends on the browser and the image server's cache configuration. I'm not proposing to rule out the use of URLs, however -- I just think data URIs would be a good way for plugin authors to reduce HTTP overhead, similar to the CSS sprite technique but without the need for CSS. So when possible, I'd like to see Orion with data-URI-friendly APIs, recognizing that data URIs are likely to be longer than regular URLs, and probably shouldn't be passed across the service bus every time. In this case, designing the API accordingly also separates presentation from content a bit more, which is nice. > I would expect that we would still support a simple string as a description > if no style information is required. Yeah, I think we should keep that too. It does come with a documentation cost for the field, but would make life easier for simple content assist providers who don't want to deal with the style-related parts of the API. +1 on the patch. I would like issues #1 to #3 raised in Comment 13 to be fixed, but that can be done in a follow-up commit. Points #4 and #5 can go into a separate bug. Thanks, Mark. I think it's best just to get this patch incorporated and then work on the improvements later. I took Tom's head revision and made a few changes to it, see the attachment. I removed the styleClass property and the nobr dom node as described in the comments. I also fixed a bunch of if tests. I'm fine with the way things are now. I am not going to push on alternate, simplified API for proposal descriptions. Thanks for the work on this, Tom. I still need you to add a comment as described in step 4 here: http://wiki.eclipse.org/Development_Resources/Handling_Git_Contributions#Git I also need the same for Bug 392073. When this is done, I will push commits for both bugs to master. Created attachment 223894 [details]
Extra changes on top of original commit
Here's the additional changes that I am making for this issue.
Also created bug 394945 amd bug 394944 for parts 4 and 5 of c13. Closing as part of a mass clean up of inactive bugs. Please reopen if this problem still occurs or is relevant to you. For more details see: https://dev.eclipse.org/mhonarc/lists/orion-dev/msg03444.html |