Dave Peticolas <[EMAIL PROTECTED]> writes:
First let me say, overall, nice job. However... :>
> Session *
> -main_window_get_session (void)
> +gncGetCurrentSession (void)
I think there may already be a function for this. In fact, I'm not
sure this is the right function. Let me poke around in my old mail...
Ahh. Here's what Linas told me. I haven't yet carefully examined
what you're doing with gncGetCurrentSession, but see if this is
relevant:
Right, but I think the correct thing to do is to not ask for the
session, but to ask for the top-most group, using
* The gncGetCurrentGroup() routine will return the account group associated
* with the current session. It will always return a non-null value
* (barring system catastrophe such as out of memory).
from FileDialog.h.
The problem with using the session directly is that one may not always
exist, so you have to scramble to figure out the semantics. By contrast,
gncGetCurrentGroup() is gaurenteed to 'do the right thing' and return the
Group object for the main window.
(The 'right thing', in particular, is if the user has started with a
clean slate, and has not yet specified a filename to save into, there is no session
yet. Maybe 'session' is a bad name, because gnucash Session is really more
like 'file lock'.)
Can you convert whatever code uses 'session' to use the current group instead?
--linas
Also, I'd I noticed you went to gncGetCurrentSession. If we're going
to be changing names, I'd much rather continue, for non-engine things,
migrating to the gnc_* names, like gnc_get_current_session. Also, any
functions named gnucash_* should eventually be gnc_*. They were named
before we(I) came up with gnc_*...
> +(new-function 'gnc:file-query-save
> + 'void "gncFileQuerySave" '()
> + "Query the user whether to save the current file.")
> +
Shouldn't we have a return value here? Eventually we want to have a
"yes no cancel" dialog, and we want cancel to be able to abort the
shutdown in cases where we're not going down hard. I know I've been
unpleasantly startled more than once when I accidentally quit the app,
it asked me if I want to save my file "yes/cancel", and I clicked
cancel, expecting the app to cancel the quit, but instead canceling
the save, as the app vanished.
> +(new-function 'gnc:file-quit
> + 'void "gncFileQuit" '()
> + "Stop working with the current file.")
Same here. If this function does anything that we might need to know
about, let's give it a meaningful return value. If not, then fine.
> +(new-function 'gnc:ui-is-running?
> + 'tSCM "gnucash_ui_is_running" '()
> + "Predicate to determine if the UI is running.")
> +
> +(new-function 'gnc:ui-is-terminating?
> + 'tSCM "gnucash_ui_is_terminating" '()
> + "Predicate to determine if the UI is in the process of terminating.")
I'd rather have the C level functions return a gncBoolean, and then
have g-wrap handle the scheme conversion via 'bool. i.e.
(new-function
'gnc:ui-is-terminating?
'bool "gnucash_ui_is_terminating" '()
"Predicate to determine if the UI is in the process of terminating.")
The reason for this is twofold. First of all, if you're going to have
to create a C function anyway, unless there's some reason to schemify
its prototype, then you might as well leave it as a good old fashioned
C function.
Second, (and this doesn't actually apply here since you're using
statics, but for general reference) I'd like to try to always use
functions to access globals (actually I'd rather not have any globals,
just statics accessed via functions). This doesn't buy us much right
now, but in the long run could really save us some headaches. If you
wrap all these statics (their getters and setters, etc.) via
functions, then you have a lot more control over what happens when
they're accessed/modified if you ever need that, and it costs you
virtually nothing unless you're in a really performance critical bit
of code, and then you can just start using inlining.
> +(new-function 'gnc:ui-shutdown
> + 'void "gnc_ui_shutdown" '()
> + "Shutdown the UI.")
> +
> +(new-function 'gnc:refresh-main-window
> + 'void "refreshMainWindow" '()
> + "Refresh the main window.")
Hmm. I wonder if we need some return values here too... I suppose it
depends on what we want to do, but depending on how we strcture
things, I could imagine that you might be able to cancel a shutdown,
though perhaps that would happen elsewhere, like in the (gui) shutdown
hooks.
> +void
> +gnc_ui_shutdown (GtkWidget *widget, gpointer *data)
> +{
> + if (gnome_is_running) {
> + if (app) {
> + gtk_widget_destroy(app);
> + app = NULL;
> + }
> + gtk_main_quit();
> + gnome_is_terminating = TRUE;
> + }
> +}
Should gnome_is_terminating be set before gtk_main_quit is called? I
know you said that it only sets some flags, but do you think it might
ever be the case that gtk_main_quit could cause other code to be
executed that might look at gnome_is_terminating? Probably not, but I
just wondered...
> int
> -gnucash_lowlev_app_main()
> -{
> +gnucash_ui_main()
> +{
> + /* Initialize gnome */
> + gnucash_ui_init();
> +
> /* Make main window */
> mainWindow();
> -
> - gtk_widget_realize (app);
> -
> +
> + gnome_is_running = TRUE;
> +
> /* Enter gnome event loop */
> gtk_main();
>
> + gnome_is_running = FALSE;
> + gnome_is_terminating = FALSE;
How about putting these two FALSE assignments in the top of
gnucash_ui_init? That seems a little more logical to me.
Also, I've been halfheartedly trying to use gncBoolean along with
GNC_F and GNC_T. The only reason to use this over TRUE and FALSE is
that these are generally only defined if you include the X libraries.
For us that's almost always true, but it's probably better to avoid
the dependency. Also using a boolen type rather than just int makes
it clear from the prototype what's going on.
> +
> return 0;
> }
> void
> gnc_shutdown (int exit_status) {
> - static int handled_shutdown = 0;
>
> if(guile_is_initialized) {
> /* This should be gh_lookup, but that appears to be broken */
> @@ -147,29 +170,15 @@
> SCM scm_exit_code = gh_long2scm(exit_status);
>
> gh_call1(scm_shutdown, scm_exit_code);
> - /* this will never execute */
> - handled_shutdown = 1;
> +
> + return;
> }
> - } else {
> - _gnc_shutdown_(exit_status);
> - exit(exit_status);
> }
> -}
>
> -/* ============================================================== */
> -
> -void
> -_gnc_shutdown_(int exit_status) {
> - /* Put any C level routines that must be called at shutdown here
> - gnc:shutdown will call this eventually...
This was originally a place for non-gui, non-guile level cleanup that
*always* needs to happen (of course that might argue in favor of
atexit() or something. Basically I intended it as a place to put
really low-level stuff. For example, if you had a call to shutdown a
memory debugger or something, that would have gone here. Where would
that stuff go now?
> + /* Either guile is not running, or for some reason we
> + can't find gnc:shutdown. Either way, just exit.
> */
> -
> - /* These calls should probably eventually be moved to the scheme
> - side... */
> - gncFileQuerySave();
> - /*XtUnmapWidget(gnc_get_ui_data());*/ /* make it disappear quickly */
> - gncFileQuit();
> - gnc_ui_shutdown();
> + exit(exit_status);
> }
> + (cond ((gnc:ui-is-running?)
> + (if (not (gnc:ui-is-terminating?))
> + (begin
> + (gnc:hook-run-danglers gnc:*ui-shutdown-hook*)
> + (gnc:ui-shutdown))))
So the gnc:ui-is-terminating? is to catch cases where the
gnc_ui_shutdown has already been called? When would that happen, and
if it could be called asynchronously via X, do we have a race
condition here?
--
Rob Browning <[EMAIL PROTECTED]> PGP=E80E0D04F521A094 532B97F5D64E3930
--
Gnucash Developer's List
To unsubscribe send empty email to: [EMAIL PROTECTED]
