[opensource-dev] Review Request: STORM-1018 Improve error messaging for External Editor feature

[Vadim ProductEngine]

> On March 7, 2011, 10:31 p.m., Merov Linden wrote:
> > I like the intent. In the code though, I don't really understand why the EC_NOT_SPECIFIED case is treated differently than other error messages. I fail to understand why this needs a "custom" message (in the panel xml instead of strings.xml) and why having it in the panel xml has any advantage.
> 
> Vadim ProductEngine wrote:
>     It's because the XUI preview tool and script editing provide different ways to specify external editor.
>     
>     Script editing: ExternalEditor setting, which can be overriden with LL_SCRIPT_EDITOR environment variable.
>     
>     XUI preview tool: ExternalEditor settings, which can be overriden with LL_XUI_EDITOR that, in turn, is overridable with the Editor Path input field of the floater.
>     
>     Thus we need different warning messages for the two cases.
> 
> Merov Linden wrote:
>     I see. I understand the need for the 2 different warning messages. Still, the implementation is rather inelegant:
>     - LLExternalEditor::getErrorMessage(LLExternalEditor::EC_NOT_SPECIFIED) though specified is actually never used
>     - the whole "if / else" switch to extract the message string in indra/newview/llfloateruipreview.cpp is actually useless since ExternalEditorNotSet is used
>     - the string ExternalEditorNotSet is defined twice (once in en/floater_ui_preview.xml and once in en/strings.xml)
>     
>     It just feels like lots of code to just get an error message from an error status.
>     
>     I'd rather create an additional error for preview (EC_NOT_SPECIFIED_PREVIEW) and overwritte the status in the preview tool using something like:
>     status = (status == LLExternalEditor::EC_NOT_SPECIFIED ? LLExternalEditor::EC_NOT_SPECIFIED_PREVIEW : status);
>     
>     Then put all the messages in one place, use one call to get the msg from the status and avoid all this "if / else" on message extracting.
>

1. Right, the getErrorMessage() is never called with EC_NOT_SPECIFIED. I just added a generic error message for consistency.
2. The if/else statement isn't useless because LLPanel::getString() and LLTrans::getString() actually look up the string in different files: the panel/floater XML and strings.xml respectively.
3. Yes, the "not specified" messages have similar keys in strings.xml and in floater_ui_preview.xml, but is as harmless as having similarly named 
local variables in different methods. However I can change the name if you find it confusing.

I agree that the code implementing custom messages is not very elegant, but I'd still prefer it to embedding caller-specific error codes to the generic LLExternalEditor class.


- Vadim


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/179/#review432
-----------------------------------------------------------


On March 7, 2011, 9:16 a.m., Vadim ProductEngine wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/179/
> -----------------------------------------------------------
> 
> (Updated March 7, 2011, 9:16 a.m.)
> 
> 
> Review request for Viewer and Seth ProductEngine.
> 
> 
> Summary
> -------
> 
> Let the user know what's wrong with external editor.
> 
> Added meaningful messages for the following errors:
> * Editor not specified.
> * Error parsing command line.
> * Specified binary not found.
> * Editor failed to run.
> 
> All the messages are translatable.
> 
> 
> This addresses bug STORM-1018.
>     http://jira.secondlife.com/browse/STORM-1018
> 
> 
> Diffs
> -----
> 
>   indra/newview/llexternaleditor.h ef2df52563bb 
>   indra/newview/llexternaleditor.cpp ef2df52563bb 
>   indra/newview/llfloateruipreview.cpp ef2df52563bb 
>   indra/newview/llpreviewscript.cpp ef2df52563bb 
>   indra/newview/skins/default/xui/en/floater_ui_preview.xml ef2df52563bb 
>   indra/newview/skins/default/xui/en/panel_script_ed.xml ef2df52563bb 
>   indra/newview/skins/default/xui/en/strings.xml ef2df52563bb 
> 
> Diff: http://codereview.secondlife.com/r/179/diff
> 
> 
> Testing
> -------
> 
> Test cases:
> 1. Use a path containing spaces without enclosing it with double quotes (/path to/editor).
>    Expected: the "not found" message.
> 2. Specify empty path ().
>    Expected: the "not found" message.
> 3. Try using an odd number of double quotes ("/path to/editor "%s").
>    Expected: the "parse error" message.
> 4. Specifying a nonexistent editor (/non/existent/editor).
>    Expected: the "not found" message.
> 5. Specify a valid editor path (/usr/bin/editor).
>    Expected: the editor is executed.
> 
> The command can be specified with the ExternalEditor debug setting or an environment variable: LL_SCRIPT_EDITOR for script editor and LL_XUI_EDITOR for the XUI preview tool. In the latter case you can also override the command via the "Editor Path" floater input field.
> 
> 
> Thanks,
> 
> Vadim
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110310/68d93f17/attachment.htm