> 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.
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. - Merov ----------------------------------------------------------- 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
