Hi April,
Congratulations, you're the lucky winner of ... a code
review! But I am not reviewing the JobQueue itself.
Instead I will focus on how the SciTE Tools Menu and the
Extensions interface with the new job system. (Mainly
because I worked on the Tools Menu and on the Extensions, so
I have a personal interest in those parts. But also because
multithreaded code is hard to review, and I am lazy.)
1) As Neil pointed out, the correct model for setting up the
function is like Open, not like SendEditor. SendEditor and
SendOutput are closures so that they can share the same
code. (A closure is just a function with some associated
data: in this case, the extra data is a string that says
where to send the message.)
lua_pushliteral(luaState, "Open");
lua_pushcfunction(luaState, cf_scite_open);
lua_rawset(luaState, -3);
lua_pushliteral(luaState, "Execute");
lua_pushcfunction(luaState, cf_scite_execute);
lua_rawset(luaState, -3);
The Lua API is stack-based, so it takes a bit of getting
used to. First you push the arguments, then you call the
function. The function call may reference other stack items
by index; that's what the -3 is. So here, if this were a
normal C-style API, it would look something like this
(pseudocode):
sciteTable = newTable()
rawSet(sciteTable, "SendEditor", wrapClosure(cf_scite_send,
"editor"))
rawSet(sciteTable, "SendOutput", wrapClosure(cf_scite_send,
"output"))
rawSet(sciteTable, "Open", wrapFunction(cf_scite_open))
rawSet(sciteTable, "Execute",
wrapFunction(cf_scite_execute))
rawSet(globalVars, "scite", sciteTable)
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.)
With this change, cf_scite_execute would become much
simpler, and would look something like this:
static int cf_scite_execute(lua_State *L) {
const char *cmd = luaL_checkstring(L, 1); // cannot return
NULL
const char *dir = luaL_optstring(L, 2, NULL);
const char *mode = luaL_optstring(L, 3, NULL);
const char *input = luaL_optstring(L, 4, NULL);
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;
}
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.)
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 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.
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.)
Best regards,
Bruce
"April White" <[EMAIL PROTECTED]> wrote in message
news:[EMAIL PROTECTED]
>I have uploaded to
>http://www.scintilla.org/aprilw/scite-april-2006-01-26.zip
>my latest changes to the job system.
>
> The Changelog.txt file contains more information. A brief
> list is:
> - an interface between Lua and the job system has been
> added as scite.execute()
> - Lua jobs can use 'command.mode' settings, though there
> are some exclusions
> - to facilitate the decoding of 'command.mode' strings, an
> extender method has been added
> - the code that replaces the current selection with the
> results of the job has been moved out of the tool thread
>
> April
>
> --
> Behind every successful woman... is a substantial amount
> of coffee.
> Stephanie Piro
_______________________________________________
Scite-interest mailing list
[email protected]
http://mailman.lyra.org/mailman/listinfo/scite-interest
