Bruce Dodson wrote:
Congratulations, you're the lucky winner of ... a code
review!
Hello Bruce. I feel so... special :-)
1) As Neil pointed out, the correct model for setting up the
function is like Open, not like SendEditor.
I've altered this in my source. Thank you Neil and Bruce.
2) If it is necessary to add new methods to the
ExtensionAPI, I would try to keep them at a high level so
that other extensions (e.g. Director) can also use them
without having to copy boilerplate code from LuaExtension.
To that end, you might consider removing DecodeCommandMode
from ExtensionAPI, and changing AddJob to do that work.
AddJob could have this signature:
virtual bool AddJob(const char *jobCommand, const char
*jobDirectory, const char *jobMode, const char
*jobInput=0)=0;
The implementation of AddJob would decode the mode string to
get the subsystem, save-before option, and flags. Pass a
non-NULL for jobInput (even an empty string) and that means
jobHasInput gets added to the flags after the mode string is
decoded. (Also, since the raw flags are not passed in to
AddJob, there is no need to mask out the flags that are
disallowed from extensions.)
I'm not sure I am following you on this.
AddJob() is a low-level interface between an extension and the tool
system. I wanted to allow the user the choice of using scite.Execute()
with either numeric identifiers for subsystem and flags or a command
mode string, much like existing tools can be configured.
To change the implementation as you suggest would restrict them to only
the mode method.
if (!host->AddJob(cmd, dir, mode, input)) {
lua_pushfstring(L, "failed to add %s to the SciTE command
queue", cmd);
raise_error(L);
}
return 0;
}
I like this. Thank you.
3) It was good to factor out code that is used by both the
new job subsystem and the tools menu. Much better than copy
/ paste. But in the process, you had to invent some new
workarounds, to shoehorn all the command properties into
flags so they could be passed around more easily; and to
continue supporting the old-style command properties
(command.quiet, command.save.before, etc).
I think the solution is to factor out even more of the code.
What if the old-style command properties are handled just by
appending them to the command.mode? What if ToolsMenu just
reads the properties, and delegates to another method to
interpret them and submit the command to the queue? (It
could delegate to AddJob, or to a method that is called from
both ToolsMenu and AddJob.)
This is an interesting proposal. Let me think on it for a bit; I'll be
away next week on business though so be patient.
Well, I had to give it a try. A sample implementation (that
does not depend on the new job system) is uploaded here:
http://users.hfx.eastlink.ca/~gisdev/SciTE_ToolsMenu_sample.zip
I've grabbed a copy. t'nks
I am not clear on what AddJob does exactly, what the return
value means, why some job subsystems are disallowed, etc.,
so I didn't try to integrate that in my sample code.
Neil did not want an extension to interface to a SciTE method such as
AddCommand(); AddJob() only has simple C parameter types
As it is meant to be used by an extension, AddJob() excludes certain
subsystems and flags that could cause thread contentions.
4) Since I peeked at AddJob, I suppose I should mention that
SaveIfUnsure prompts the user with a modal dialog. If
AddJob is called from a thread other than the main UI
thread, the dialog would probably appear non-modal. Might
be worth testing. (You won't be able to test it from Lua,
because Lua code always executes in the main thread.)
Rest assured that Neil has been all over me about the code being thread
safe. I left the call to SaveIfUnsure() in but I believe by excluding
the 'savebefore:prompt' command mode, this method will never be called.
Thank you Bruce for your review and insights. I'll be keeping your
email marked as 'unread' so I will re-read it when I return from the States.
April
--
As I learn to trust the universe, I no longer need to carry a gun.
_______________________________________________
Scite-interest mailing list
[email protected]
http://mailman.lyra.org/mailman/listinfo/scite-interest
