> 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 > >
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
