Dev opinions needed: when to activate SPDesktops?
Hi all,
during my investigation for the cause of bug #195373 [Creating a new document breaks a 3D box] I have found that when a new desktop is created, it isn't automatically set active right away.
Namely, in sp_desktop_widget_new() the function inkscape_add_desktop() is called, but in this function the new desktop is *appended* (instead of *prepended*) to the list of existing desktops. Thus the following 'if' construct
if (DESKTOP_IS_ACTIVE (desktop)) { ... }
is not triggered (except for the very first desktop). I wonder if this is intentional. Well, it probably is, but why? :) Is there any reason why new desktops shouldn't be activated upon creation?
The current behaviour is what's causing bug #195373 since the perspectives in the _old_ desktop/document are updated according to the toggle states of toolbar buttons in the _new_ desktop.
There might be other ways to fix this but the easiest I have found is to simply change
g_slist_append (inkscape->desktops, desktop);
to
g_slist_prepend (inkscape->desktops, desktop);
in line 891 of inkscape.cpp. However, this would render the above-mentioned 'if' construct obsolete and I guess that there was a reason for introducing it in the first place. But since inkscape_add_desktop is not called from any other place except sp_desktop_widget_new() it looks like a rather safe thing to do.
I committed this change to SVN in rev. #17510. Please tell me if there are any objections or concerns that this could break other things. I will then revert it and try to find a different fix. Otherwise this should go into 0.46 as well (I'll file a bug in LaunchPad as soon as I know that it's safe to do).
Thanks, Max
On Tue, Feb 26, 2008 at 10:02 AM, Maximilian Albert <Anhalter42@...173...> wrote:
if (DESKTOP_IS_ACTIVE (desktop)) { ... }
Well, this check is there from Sodipodi times (actually since the very first version of that file in September 2000, as Sodipodi SVN shows). I have no idea what it is supposed to achieve. Let's test this thoroughly before moving to 0.46. All kinds of scenarios with creating and deleting desktops in different tools and modes should be tested.
-----Original Message----- From: inkscape-devel-bounces@lists.sourceforge.net [mailto:inkscape-devel-bounces@lists.sourceforge.net] On Behalf Of bulia byak Sent: dinsdag 26 februari 2008 21:27 To: Maximilian Albert Cc: inkscape Subject: Re: [Inkscape-devel] Dev opinions needed: when to activateSPDesktops?
On Tue, Feb 26, 2008 at 10:02 AM, Maximilian Albert <Anhalter42@...173...> wrote:
if (DESKTOP_IS_ACTIVE (desktop)) { ... }
Well, this check is there from Sodipodi times (actually since the very first version of that file in September 2000, as Sodipodi SVN shows). I have no idea what it is supposed to achieve. Let's test this thoroughly before moving to 0.46. All kinds of scenarios with creating and deleting desktops in different tools and modes should be tested.
Max, perhaps you can track down this bug? https://bugs.launchpad.net/inkscape/+bug/183621
Regards, Johan
J.B.C.Engelen@...1578... schrieb:
Max, perhaps you can track down this bug? https://bugs.launchpad.net/inkscape/+bug/183621
I already had a look at it (several, actually) some time ago but wasn't able to solve it at that time so I'm not too confident. But I'll give it a further look when I find the time (may not bee to soon, though).
Max
bulia byak schrieb:
On Tue, Feb 26, 2008 at 10:02 AM, Maximilian Albert <Anhalter42@...173...> wrote:
if (DESKTOP_IS_ACTIVE (desktop)) { ... }
Well, this check is there from Sodipodi times (actually since the very first version of that file in September 2000, as Sodipodi SVN shows). I have no idea what it is supposed to achieve. Let's test this thoroughly before moving to 0.46. All kinds of scenarios with creating and deleting desktops in different tools and modes should be tested.
I have been doing a bunch of tests by creating new desktops in all tools, both by creating new documents and by duplicating existing views (are there other methods)? Result: No problems at all.
Actually, I think the only question is if there is any scenario when new desktops shouldn't be activated right away. From what I understood by skimming through the rest of the code, they are activated but only a bit later, but the signals emitted are the same and I can't see any reason why emitting these slightly earlier should do any harm. In fact, as the bug shows, the contrary seems to be the case because in the intermediate time some tools may query data from the wrong desktop (namely, the one that was activated before the new one was created).
So in my opinion it should be safe to remove the check but of course further testing should be done by other people.
Max
On Feb 29, 2008, at 8:19 AM, Maximilian Albert wrote:
Actually, I think the only question is if there is any scenario when new desktops shouldn't be activated right away. From what I understood by skimming through the rest of the code, they are activated but only a bit later, but the signals emitted are the same and I can't see any reason why emitting these slightly earlier should do any harm. In fact, as the bug shows, the contrary seems to be the case because in the intermediate time some tools may query data from the wrong desktop (namely, the one that was activated before the new one was created).
I think the real problem is that that ACTIVE macros are used at all. In the past I think Ted had also expressed a desire to remove SP_ACTIVE_DESKTOP and its friends. I definitely feel that way.
One problem is that you break encapsulation. A component or widget being used says "hey, I know who should be calling my from up higher, so I'll peek out and find him". Among all the other problems, this also makes things hard to use. You might pass in a few parameters, but without inspecting the code it is hard to see what the bit will use or go looking for.
Another problem is that this hardcodes the assumption that the thing is a singleton being used for all desktops. This is exactly the sort of thing that started getting in the way of the dockable dialogs. A floating dialog version of a panel still should operate on the "active" desktop's document, however all embedded ones should work on one and only one desktop (the one it is embedded in). At the moment this problem can be worked around by setting the active desktop for those as active is changed, but you might get race conditions (like might be the bug here), and you can also see things like docked dialogs showing info on windows they are not actually docked in.
As Ted pointed out a while back, verbs have the desktop they are called from as a parameter... so that should be passed on down and no SP_ACTIVE_DESKTOP, IS_ACTIVE, etc, should be called. As functions call sub-functions, instead of the desktop they can pass on in things like the document, an object, etc. Limiting the type to the "smallest" and most atomic needed.
So instead of having code "poll" to find out what the active thing of the moment is, things should have the change in active desktop "pushed" down into them. You might ask "what about the floating dialogs?". That is simple. The *dialog* holding the panel should be the one watching for "active" changes, and pushing that down into the panel as it goes.
One additional refinement is that I think we need to get some "contexts" that contain some data and an interface to get at that data. At the moment we have "the whole big application", "the whole editing window" and "the whole document". I'd like to see some that are broken down to a finer degree. In fact, with the tablet support I've got need of that already. As you switch what device you use on the tablet, we switch tools. It would be nice to have a clean, generic way to also switch in a subset of settings. That way the tip of the pen could be the calligraphy tool set for drawing, and the back could be the same tool but set for engraving, etc.
participants (4)
-
unknown＠example.com -
bulia byak -
Jon A. Cruz -
Maximilian Albert