Community
Participate
Working Groups
Hi, in the attachment you can find my proposals for the ridgets. Todos for me are marked with TODO. Best thanks for hints.. Flo
Created attachment 132501 [details] Ridgets
Flo, thanks for the contribution. I’ll provide feedback after M7 (=next week).
Hi Florian, first thanks a lot for your contributions! Overall it looks very good. A couple of conceptual questions and minor points are listend below. Don't be scared by the lengthly response, Most of the points are very minor and I tried to be thorough :-) I've also created a fragment for your code. It seems the easiest way to share runnable code until it's committed. Kind regards, Elias. Conceptual questions a. LinkRidget: do you see value in having several links per link ridget? At least the SWT Link widget supports that, i.e. "<a>foo</a> and <a>bar</a>" b. LinkRidget: can I do something like "Click <a>here</a>" ? c. BrowserRidget: why does it rely on LinkInfoMergeConverter.LinkInfo ? I would just expect to bind to a String or a URL value in in bindToModel(...) Legal stuff 1. Please add EPL copyright headers on all .java files that have no header. See http://www.eclipse.org/legal/copyrightandlicensenotice.php for details. 2. Please add yourself as an author (under contributors) for files derived from riena: /******************************************************************************* * Copyright (c) 2007, 2009 compeople AG and others. ... * Contributors: * compeople AG - initial API and implementation * your name - ... what you did ... *******************************************************************************/ Comments 3. All new API (=interfaces) should have a '@since 1.2' javadoc annotation on the type. * @since 1.2 */ public interface IBrowserRidget extends IRidget { 4. I added createMarkerSupport() to enable correct marker support (something we added after your contribution) 5. Wrote some tests to try out the ridgets. 6. Obviously the package names will change once this goes into Riena. 7. In the ridgets bundle I follow a convention that puts the private methods last. Code Review LinkRidget.java - The LinkRidget(Link) constructor is not needed. Typically we only have a parameterless contructor: public LinkRidget() { linkBinder = new LinkBinder(); actionObserver = new ActionObserver(); textAlreadyInitialized = false; } - initText() - this is there to preserve the widget text if the Ridget text is null. Typically the case when somebody does button.setText(...) in the View and does not use buttonRidget.setText(...) in the controller. Not sure if it's needed for the LinkRidget. I would propably NOT support until someone explicitly requests it... - setLinkText() - should be firing PROPERTY_LINK_TEXT, not PROPERTY_LINK - fixed - setLinkText() - was using the 'link' value as the old value - fixed String oldValue = this.getLink(); now: String oldValue = this.getLinkText(); - each inner class should have a short javadoc (i.e. one or two sentences explaining what LinkBinder, DefaultInfoMergeConverter are good for) - getLinkInfoMergeConverter() - if we keep it (see below), it should propably become an interface method. Also because of symmetry - shouldn't it be named getLinkMergeConverter() ? - Personally, I think the LinkRidget would be more flexible without the LinkInfoMergeConverter. Just using the linkTextConverter and the LinkConverter and removing the limitation that the From-Type has to be LinkInfo - just my thoughts... - unbindUIControl() must invoke super.unbindUIControl() - fixed ILinkRidget.java - get/setLink* Methods - parameter / return value - can it be null? - bindToModel(a, b, c) - I preffer a four argument method, so it's more consistent with the other ridgets, i.e. bindToModel(linkValueHolder, linkPropertyName, linkTextValueHolder, linkTextPropertyName). This will work when the two 'holder' objects are the same or differnt. Also we avoid using the words pojo or bean, because bindToModel should work with both. - setLinkMergeConverter(...) - can you explain the use case for this? It is not clear to me why it's needed. - type javadoc: the statement that the link ridget is readonly and does not modify the model is not true. See LinkRidgetTest#testBindToModel(). LinkInfoMergeConverter.java - why is this interface needed? I'm asking because it does not any additional methods to the IConverter interface. - LinkInfo: why is this needed in the interface? It is only used internally (at least in the contributed code). IBrowserRidget.java: - set/getUrl - parameter / return value - can it be null? - bindToModel: params should be named valueHolder, urlPropertyName - we avoid pojo or bean - setModelToUiControlConverter - for consistency it should be: setModelToUIControlConverter - type javadoc: mentions Link - looks like a copy-paste artefact... - type javadoc: the statement that the browser ridget is readonly and does not modify the model is not true. See BrowserRidgetTest#testBindToModel(). BrowserRidget.java - No need for BrowserRidget(Browser) constructor - initText() not needed since the browser shows no text - updateUIText() should be renamed to updateURL() - setModelToUIControlConverter: why not Assert.isNotNull(modelmodelToURLConverter) instead of ensureModelToUIControlConverter? - setUrl(...) - do you think checking the input makes sense? For example it could invoke new URL(String) which throws a malformed url exception... - unbindUIControl() must invoke super.unbindUIControl() - fixed - Constructor: must invoke valueBindingSupport.setMarkable(this); - fixed - When writing the unit tests I found that the Browser does not support focus events which makes the setFocusable(t/f) unusable. I've added this to the javadoc of the ridget. ---
Created attachment 136872 [details] Fragment v1 - Converted contribution to fragment (File > Import... > Existing Projects into Workspace > Archive file) - Added Tests - Minor fixes
thanks elias for the detailed review of flos first contributions, (In reply to comment #3) > ......> > Conceptual questions > > a. LinkRidget: do you see value in having several links per link ridget? At > least the SWT Link widget supports that, i.e. "<a>foo</a> and <a>bar</a>" > from my POV I would say that only one link per ridget makes sense in business applications. > b. LinkRidget: can I do something like "Click <a>here</a>" ? > We use always "Labeled Entries" for these cases, so I would have a Label "Click" besides the link --- the other parts flo will discuss with you. perhaps it will take some days because we're in the process of finishing the redView -> riena-UI-Navigation integration to make it all run and publish first beta soon. thx again ekke btw: the day comes near where I'll contribute first things (Validators, Converters) ;-)
hi elias, thanks for your answer and hints. I'll answer the questions and implement the changes at the end of coming week. greetings flo
Hi, if it is ok for you, i would like to wait for the feedback of the TraverseRidgets before i finish this contribution. So i could directly use these informations for this contribution. Greetings, Flo
Hi Florian, I'm back from vacation now. I'll review the patch for TraverseRidgets in the next two weeks. In the meantime: we are interested in including this contribution as soon as possible (i.e. next week). We could use the link ridget ourselves. In order to speed things up, I can offer to address the things from comment #3 myself. However I need your help with the copyright headers, since for legal reasons I can't do this myself. Would it be possible to address (1) and (2) from comment #3 and attach an updated fragment? Thanks for you help, Elias.
Hi Elias, sorry, i did not know about it. If i understood it right, i should finish the following points for the LinkRidget and post the fragment: 1. Please add EPL copyright headers on all .java files that have no header. 2. Please add yourself as an author (under contributors) for files derived from riena. Is the BrowserRidget also urgent? If not, it would be great if i could finish the Browser myself. If it is possible to finish it myself, can i wait for the response to the TraverseRidgets (specially to your comments of java doc) or should i finish it immediately? I'll finish 1 & 2 and post it tomorrow in the morning. Greetings, Flo
No problem - this is a new development. I did also not know that we need the Link ridget until yesterday ;-). As far as I know the Link ridget is the most important one -- @Christian: feel free to comment. Greetings, Elias.
Created attachment 143739 [details] fragement v2 Hi Elias, as written yesterday the fragment with 1 and 2. But i could provide the finished ridgets until coming week. Maybe thursday or friday. Do you want to implement the LinkRidget yourself or should i do until coming week? Greetings, Flo
Hi Flo, I would still like to do the LinkRidget myself (I could do it on Mo), so it can be included as quickly as possible. The copyright headers you created are fine for my purposes. However you should know that typically if you are submitting a file 100% written by you, you can claim full copyright yourself (unless you do so as part of a contract / employment - then in most cases these contracts assing copyright to the party paying you). We already include "and others" in our header as a simplification, however my understanding is that it only need to be there if there are multiple copyright holders. If the above applies to you, a typical header would look like the one below. Feel free to submit and updated fragment if you like. See this link for more info: http://www.eclipse.org/legal/copyrightandlicensenotice.php Example: ILinkRidget.java: /******************************************************************************* * Copyright (c) 2009 Florian Pirchner. * All rights reserved. This program and the accompanying materials * are made available under the terms of the Eclipse Public License v1.0 * which accompanies this distribution, and is available at * http://www.eclipse.org/legal/epl-v10.html * * Contributors: * Florian Pirchner - initial API and implementation *******************************************************************************/ Kind regards, Elias.
sorry Elias, I oversaw that were pinging me in #10. Yes the LinkRidget is the most urgent one. @Florian its nice that you give the copyright to compeople, but probably not the right path. As Elias noted, you probably want to claim copyright for yourself. I propose that Elias starts with the current patch and that he changes the copyright from compeople to Florian Pirchner. That is ok with me for all classes in this patch. The next milestone is coming hopefully early next week and we like (if possible) the LinkRidget to be in it.
Hi, I'll submit changed copyrights this evening. I wasn't sure, because in this special case i have developed ridgets which are internally based on other ridgets.. So i have written code, which is based on the ideas of compeople. Of course I really like the idea, that i have the copyright. Thanks Christian to make things clear. Greetings, Flo
Created attachment 144115 [details] fragment with new copyrights
Created attachment 144170 [details] ILinkRidget from Florian Pirchner adjusted for Riena 1.2. Hi Flo, thanks for the update. Based on your contribution I've created a patch with the LinkRidget than can go into Riena 1.2. I've addressed the open issues and performed some additional changes to make things more similar with existing APIs and re-use more existing code. For example some method names have changed, ILinkRidget extends IActionRidget and LinkRidget extends AbstractActionRidget. These are new things that have been created since the original review. If you have strong feelings about any of these changes feel free to comment. My only remaining question is related to the LinkMergeConverter: if (StringUtils.isDeepEmpty(ridget.getText())) { result = String.format("<a>%s</a>", ridget.getLink()); //$NON-NLS-1$ } else { result = String.format("<a href=\"%s\">%s</a>", ridget.getLink(), ridget.getText()); //$NON-NLS-1$ } What I find interesting is that it uses the link when text is empty, but does not consider an empty link with non-empty text. Personally I think it would make more sense to always show the link-text (i.e. ridget.getText()) underlined: if (StringUtils.isDeepEmpty(ridget.getLink())) { result = String.format("<a>%s</a>", ridget.getText()); //$NON-NLS-1$ } else { result = String.format("<a href=\"%s\">%s</a>", ridget.getLink(), ridget.getText()); //$NON-NLS-1$ } Any thoughts on this? Thanks again for your contribution, Elias.
Hi Elias, you are completely right. So the ridget would have the same behavior independent whether a link is defined or not. And the user does not see different representations. Of course, i also prefer your idea! ;-) But what should happen, if a link is defined and the linkText is empty? It could be an idea that the linkRidget only has a text representation if a text is specified! (I would prefer this.) Or the link could be used as its text. But everybody can easily change this behavior on writing his own LinkMergeConverter. What do you think? Greetings, Flo
CQ 3486 submitted
Comment on attachment 144115 [details] fragment with new copyrights Great, the contribution has been approved.
Hi Florian, thanks for the contribution. Please post a bugzilla comment to confirm that: 1. You authored 100% of the contribution (except the test cases written by me) 2. You have the rights to donate the content to Eclipse Thanks, Elias.
Created attachment 144337 [details] ILinkRidget from Florian Pirchner adjusted for Riena 1.2 - v2 Christian, I've reworked the patch based on our discussion this morning. From my POV it can be committed as soon as Florian confirms comment #20. Florian, FYI here's an overview of the main changes and enhancements: - LinkRidget has been simplified significantly - it only has getText(...) / setText(...) which expect data in the widget's format (i.e. <a>foo</a> or <a href="...">...</a>). This also eliminates the need for merging and conversion. If necessary this still can be done by binding to a special bean that merges / converts (see snippet #2). - it supports partial links per LinkRidget - i.e. "Click <a>here</a> to do something" - it supports multiple links per LinkRidget - i.e. "Click <a>here</a> or <a>there</a>". To support this the IActionListener was changed to ISelectionListener. The list argument of the selection listener contains the segment clicked - the Snippets have been updated to demo these things Kind regards, Elias.
Hi Elias, I can confirm that: 1. I authored 100% of the contribution (except the test cases written by you) 2. I have the rights to donate the content to Eclipse Thanks again for your great help!!! Florian
Created attachment 144887 [details] ILinkRidget from Florian Pirchner adjusted for Riena 1.2 - v3 Added Link sample to the Playground
Hi Florian, just to give you a quick update: I am still waiting for the final legal approval from the Eclipse Foundation. Unfortunatelly this is a slow process. We are at position 20 or so in the queue :(. Ekke should be able to view the status at: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=3486 (I don't know why, but only committers have access.) Regarding the browser ridget: my suggestion is that I add this to v3 of my patch based on what you have submitted in 'fragment with new copyrights'. This way we don't have to go through another review for that. Kind regards, Elias.
Hi Elias, of course. I am not so familiar with the approval process. Please tell me if i should finish the browser ridget immediately. I thought i'll wait for the review of the TraverseRidgets. I hope that the TraverseRidgets will fully meet your expectations... ;-) I took a lot of care and tried to figure out all the possible combinations. Pleas tell me, if i should finish it currently. Greetings, Flo
Created attachment 145741 [details] ILinkRidget and IBrowserRidget from Florian Pirchner adjusted for Riena 1.2 - v4 The latest patch has both Link and Browser. Good to go as soon as we get 'checkintocvs' from the IP Team... Florian, just some background info: I've addressed the comments regarding the Browser ridget myself. It was straightforward. I'm currently waiting for approval of 'fragment with new copyright'. If you had done any changes, I would have to start over with the latest fragment. I'll look at your other contribution next.
Created attachment 145742 [details] mylyn/context/zip
Hi Elias, no, currently there are no changes contained in the fragment except the copyright. Greetings, Flo
Received IP approval. Committed to HEAD. Thanks Florian!
