Bug 361900

Comment 1 pdu 2011-10-25 05:19:40 EDT

Created attachment 205891 [details]
Patch to implement Variables in outlets

This patch enables variable handling for outlets. It opens a StringVariableSelectionDialog

Also corrected a minor bug allowing empty path on outlets when creating them

Also added javadoc comments

Comment 2 Romain Sezestre 2011-10-25 05:44:15 EDT

Ali, could you please review proposed patch and commit it to Sphinx trunk if approved? Thanks!

Comment 3 Stephan Eberle 2011-10-27 03:48:18 EDT

Thank you for this contribution. I appreciate the fact that on top of providing the new feature you have spent time on adding missing Javadoc. This is greatly appreciated!

I did a quick code review, and here are some remarks from my side. Please honor them when incorporating the patch into the Sphinx codebase:



* EditOutletDialog(Shell parent, ExtendedOutlet outlet, boolean edit, boolean editableName, OutletProvider outletProvider):

Please adjust the comment, from "Default class constructor" to "Constructor". The default constructor is the constructor that has no parameters.



* handleInsertVariable():

Why is it necessary to use the clipborad here? Wouldn't a simple statement like

  locationText.setText(variableName) 

be sufficient?



* validateOutletLocation():

The comment "isValidPath method is really dumb (only checks that ':' is not present in path segments)" is not appropriate and should be removed. In addition to ':' isValidPath() also looks at misplaced '\' and '/' depending on underlying operating system - so it's not as dump as it might appear.



* General remark:

Please make sure that // comments always start with an upper case letter and Javadoc comments are always full sentences with an upper case letter at the beginning and a period at the end.

Comment 4 pdu 2011-11-15 05:57:48 EST

(In reply to comment #3)
> Thank you for this contribution. I appreciate the fact that on top of providing
> the new feature you have spent time on adding missing Javadoc. This is greatly
> appreciated!
> 
> I did a quick code review, and here are some remarks from my side. Please honor
> them when incorporating the patch into the Sphinx codebase:
> 
> 
> 
> * EditOutletDialog(Shell parent, ExtendedOutlet outlet, boolean edit, boolean
> editableName, OutletProvider outletProvider):
> 
> Please adjust the comment, from "Default class constructor" to "Constructor".
> The default constructor is the constructor that has no parameters.
> 
> 
> 
> * handleInsertVariable():
> 
> Why is it necessary to use the clipborad here? Wouldn't a simple statement like
> 
>   locationText.setText(variableName) 
> 
> be sufficient?
> 
> 
> 
> * validateOutletLocation():
> 
> The comment "isValidPath method is really dumb (only checks that ':' is not
> present in path segments)" is not appropriate and should be removed. In
> addition to ':' isValidPath() also looks at misplaced '\' and '/' depending on
> underlying operating system - so it's not as dump as it might appear.
> 
> 
> 
> * General remark:
> 
> Please make sure that // comments always start with an upper case letter and
> Javadoc comments are always full sentences with an upper case letter at the
> beginning and a period at the end.

Dear Stephan,

I really appreciated your remarks! I will reply to your question about clipboard use in handleInsertVariable() : my understanding of variable use is to be able to mix 'hard-typed' text and one or several variables, for example having "some text ${var1} some other text ${var2}"; I didn't think about full text substitution, and in this case, your solution is obviously the best !

Comment 5 Ali AKAR 2011-11-15 08:39:30 EST

I Fixed the bug by applying the patch provided in Comment #1, I also took into account Stephan's remarks (Comment #3).

Comment 6 Balazs Grill 2021-07-14 02:17:24 EDT

Mass-closing Resolved tickets