[ jEdit-devel ] [ jedit-Patches-2964432 ] Docking Framework Patch

[SourceForge.net <no...@so...>]

Patches item #2964432, was opened at 2010-03-05 13:19
Message generated for change (Comment added) made by kog13
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=300588&aid=2964432&group_id=588

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Damien (kog13)
Assigned to: Shlomy Reinstein (shlomy)
Summary: Docking Framework Patch

Initial Comment:
In order to help make saving and restoring docking layouts easier, I created a patch that does several things:

1) Changes .xml files saved in .jedit/DockableWindowManager to save a comma-separated list of dockables at each position, with the current dockble surrounded by square brackets, e.g. BOTTOM=error-list,task-list,[console]. This allows layouts to actually save an entire docking layout, rather than just the current dockable at each position.
2) Added save and restore layout buttons to the general option's docking pane to enable explicit saving and loading

I would like to simply commit my changes, but before I do that I need to extend it to work with frameworks other than jEdit's default framework and do a little additional debugging, but I wanted to release this so that I can get some feedback (particularly on the layout of the docking option pane).

I'm also doing this in order to make setting preferred layouts for plugin packages more viable, when they are eventually implemented.

As a side note, I think docking config files in the settings folder should be saved a level deeper, such as in ~/.jedit/wm. It's more of an aesthetic reason than anything, but having a folder called DockableWindowManager surrounded by dtds, jars, and macros looks strange to me. I would prefer a lowercase, shorter name convention in the settings folder. IMO, ~/.jedit/wm/DockableWindowManager is better than ~/.jedit/DockableWindowManager.

----------------------------------------------------------------------

Comment By: Damien (kog13)
Date: 2010-10-24 01:38

Message:
I uploaded the new version of the patch. I haven't updated the README yet,
but if this version gets approved, then I'll update it and submit the final
patch.

Let me know if there are any other concerns or if I've missed anything.
Otherwise, I believe I've covered everything.

----------------------------------------------------------------------

Comment By: Shlomy Reinstein (shlomy)
Date: 2010-10-15 14:37

Message:
Maybe there is no need. The layout will be set successfully the first time
from the properties; the most noticeable (in my opinion) difference from
the last layout that the user saw is that the current dockable in each area
may be different. Since this only happens once, I guess this is okay
without a converter or dialog.

----------------------------------------------------------------------

Comment By: Damien (kog13)
Date: 2010-10-15 12:51

Message:
The dockable window manager's config class now defaults to initializing
window lists and sizes based on the properties, but loading a layout resets
it and then sets everything based on the new layout. Effectively, this
means that if a perspective is saved, it is used; otherwise, everything is
set based on the properties.

On a related note, I discovered that the PanelWindowContainer class has a
save() method that sets the dimension and current open window properties,
but was never called from anywhere, which means they were never saved as
user properties even though there was a method to do so. I don't know if
this was intentional, but I fixed this in order to make sure those
properties are indeed saved.

Since the layout can now be set successfully without a perspective file
saved, is there a need for a converter or dialog?

----------------------------------------------------------------------

Comment By: Shlomy Reinstein (shlomy)
Date: 2010-10-14 14:19

Message:
3. I don't see any reason to copy the files from the old directory to the
new one, since the format has changed. The file in the old directory can
either be ignored, or be converted. And yes, what you wrote is exactly what
I have in mind. Note that before your patch, the layout that users got was
a mixture of properties and the perspective file - the perspective
specified the sizes of the docking areas, and the "current" dockable in
each area, while the properties specified which dockables are docked in
which areas. If you take the extra effort of converting the layout to the
new version, then take just a bit more effort and make it complete using
the properties. If you don't do this, you should completely ignore the
perspective and start a new layout using the properties only (and
preferably show a dialog explaining this).

4. Yes, the layout would override the properties. Note that the word is
"override", not "overwrite". MyDoggyPlugin (as well as DockingFramesPlugin)
do not change the properties according to the loaded layout. There are
various reasons for that, for example, a layout might not have a clear
separation of docking areas.


----------------------------------------------------------------------

Comment By: Damien (kog13)
Date: 2010-10-14 14:00

Message:
1. I believe the PROPERTIES_CHANGED message is still necessary. When I
tested it, removing PROPERTIES_CHANGED and attempting to load a blank
docking layout left the windows there; attempting to run something in
console resulted in a NPE with the console process, which I assume means it
was successfully deactivated, but the windows stay there unless
PROPERTIES_CHANGED is sent as well.

3. I'm seeing this as two problems: (1) the new location for saved
layouts. I think a good way to solve this would be to add a check in
DockableWindowManager's getSavedLayouts() method that checks to see if an
old directory is still present, and if any files are saved in it. If it
finds some, prompt the user (?) and copy all of the files over to the new
folder. (2) the new format for the default docking framework. This would
probably be a check in DockableWindowManagerImpl's loadLayout() method;
what I'm considering is, if the file is in the old format, skip the default
behavior in favor of either converting the file directly, or simply taking
the new layout from the properties. Is this roughly what you had in mind?

4. Would't the layout override the properties? If a file is not found,
load a layout from the properties; if one is found, load that layout and
set the properties accordingly. Is there a special case where this wouldn't
work?

----------------------------------------------------------------------

Comment By: Shlomy Reinstein (shlomy)
Date: 2010-10-14 01:45

Message:
1. Great, thanks. Does that replace the PROPERTIES_CHANGED message, or is
it sent after PROPERTIES_CHANGED? I'm not sure, but I think the
PROPERTIES_CHANGED is not needed in that case.
2. Ok.
3. A much better way to do it would be to include a converter - this can
be a separate class that just checks the existing XML (even without parsing
it as XML) to see if it's the new or old format, and if it finds the old
format, it automatically converts it to the new one, creating the new file
in the new directory, before the the last saved layout is read by the
current code.
4. You can see an example in MyDoggyPlugin.
MyDoggyWindowManager.applyDockingLayout(). This method checks if there's a
layout file that was previously saved by MyDoggy. If the file exists, the
method loads the layout from it. If the file does not exist, it calls the
inherited applyDockingLayout() method, which simply creates the dockables
according to the user properties. So, when you use MyDoggy for the first
time, you will get a layout that is relatively close to the one you had
before MyDoggy. Of course, it doesn't convert the actual sizes, but at
least it will open the list of dockables that were previously open and in
the same positions.

----------------------------------------------------------------------

Comment By: Damien (kog13)
Date: 2010-10-13 17:03

Message:
1. I added a DEACTIVATED edit bus message to DockableWindowManagerImpl's
undockDockableWindow() method.

2. I'll update the README once everything else is squared away.

3. An in-editor popup is the only way I can think of to ensure that
everyone who updates is aware of the reason their docking layout reset
itself. Not sure how well that would go over, but there could be a check
for an old docking layout folder, and if it exists then display the dialog.

4. I'm assuming you're talking about a conflict between the default
perspective layout and the user properties? I assume the perspective layout
would take priority, but I'm not sure how jEdit handles that right now.

----------------------------------------------------------------------

Comment By: Shlomy Reinstein (shlomy)
Date: 2010-10-11 23:50

Message:
1. I don't think that sending a DEACTIVATED message is unnecessary. It
pretty much depends on the usage of these messages by plugins, and I recall
that there are heavy plugins that will not get rid of their data without
seeing this message. Maybe you don't have such plugins installed or didn't
check the memory allocation.
2. The README can be updated and included in the patch. It doesn't have to
be a 2-phase commit.
3. I think there might be a problem with backward compatibility. jEdit
users currently have a saved layout (even if they don't use mode-specific
layouts or any manually saved layouts), which includes the last layout they
had when jEdit was exited. When starting jEdit with your patch applied,
what will the layout be? I think you're patch is not backward-compatible
with that, but I'm not sure that a README note is enough. It may be very
irritating for a user to start an updated version of jEdit and find out
that the dockables he had last time are not there. Need to come up with a
solution.
4. Now, when the docking position of each dockable is also specified in
the XML, there's a conflict with the properties. Each dockable also has a
property in the user properties file which specifies its docking position.
I guess that in normal circumstances, they will be the same as long as the
user does not manually save layouts, as writing the layout will use the
same up-to-date positions as the properties. Need to think what happens in
case where they are not the same. A similar problem also exists with the
pluggable docking framework (e.g. MyDoggyPlugin), and what I did for that
was that the framework's layout determines the initial dockable positions,
but changes to the properties (after startup) are also reflected in the
framework - so that any plugins / macros / whatever that have always used
the properties will continue to work.


----------------------------------------------------------------------

Comment By: Damien (kog13)
Date: 2010-10-11 11:58

Message:
With the talk going around of releasing 4.4, I wanted to get around to
finishing up this patch so that it can hopefully be included soon.

I went back and implemented most of your comments, although sending a
DEACTIVATED message when undocking a window seems unnecessary. Unneeded
dockables are undocked by calling undockDockableWindow(), which sends its
own edit bus message, and in my tests dockables are removed just fine when
loading a new layout that doesn't include them.

I also did some code cleanup, made things more consistent, and removed the
option for backwards-compatible folders. The README hasn't been updated,
but I can do that when this patch is accepted.

----------------------------------------------------------------------

Comment By: Damien (kog13)
Date: 2010-06-19 21:23

Message:
Since the size belongs to each panel and not the dockables on those panels,
I updated the patch to include four <panel> tags in addition to the list of
dockables. The dockable tags denote each window, its location, and whether
or not it's visible (defaulting to false); the panel tags remember which
size should be used when one of it dockables is shown.

This should let users create a docking layout that remembers a panel's
size, regardless of if any dockables are visible there.

----------------------------------------------------------------------

Comment By: Shlomy Reinstein (shlomy)
Date: 2010-06-12 14:09

Message:
I don't think it will be enough. Users typically adjust the size of a
docking position according to the current dockable in it. If they hide that
docking position (usually for allowing more space to the text area), they
expect to restore the size later when they show it.
Is it a problem to implement? I believe it should be simple, as this was
supported before your patch.

----------------------------------------------------------------------

Comment By: Damien (kog13)
Date: 2010-06-12 13:02

Message:
I added a small fix that sets sizes to a default value if none is set for
any dockable. Will this be enough? I don't see why it would need to be more
complicated than that.

----------------------------------------------------------------------

Comment By: Shlomy Reinstein (shlomy)
Date: 2010-06-12 02:17

Message:
there's one more issue I forgot: If none of the windows are visible in a
docking position (i.e. there are several windows at that position, but the
button of the visible one was clicked and the entire position was
minimized), the size of the docking position won't be registered in the XML
with this patch.
Note that the size belongs to the position, not to the dockables. Even if
no dockables are currently visible in a position, the size is needed later
when one of the dockables is made visible.

----------------------------------------------------------------------

Comment By: Shlomy Reinstein (shlomy)
Date: 2010-06-12 01:57

Message:
I reviewed the patch (code-only, no testing). I have a few small comments.

Functional:
- I think the backward compatibility won't work, since you've changed the
format and the old format won't be parsed correctly, am I right? If so,
then:
1. This patch must include a change in README explaining that the docking
layouts will have to be recreated.
2. There is no need to continue supporting the old directory of the layout
files (without "wm" in the path), as the old files cannot be read anyway.

- I think that DEACTIVATED message should be sent instead of
PROPERTIES_CHANGED when a layout is loaded and some dockables are undocked
as a result. The undocked ones do not remain visible, right? So they should
get DEACTIVATED message.

Style:
- In "saveLayout" - there are 4 "for" loops with very similar code, why
not write a method to take the varying parameters and just call it 4 times
instead of those loops?
- Use "Integer.valueof(String)" instead of "new Integer(String)" when you
want to convert the size from the XML file to an integer. Doesn't matter
much, but both options are the same for the code and one is better at
runtime.
- Why change the position strings from uppercase to lowercase? I have no
problem with that, but I see no reason to change it.

Coding conventions:
- Please get rid of unneeded code instead of commenting it out. (e.g. the
method 'attribute' is no longer needed at all).
- Do not use blocks (braces) where the body of "if" / "for" / ... has a
single line.
- Put braces on their own line, not on the same line as the "if" / "for" /
...


----------------------------------------------------------------------

Comment By: Alan Ezust (ezust)
Date: 2010-06-08 00:05

Message:
Shlomy, since you're the docking framework dude, it's your call whether to
accept or reject.


----------------------------------------------------------------------

Comment By: Damien (kog13)
Date: 2010-03-17 13:54

Message:
True, but I believe that there should be some indicator in the Docking
general options that users can manually load and save them. Maybe some
buttons linked to the existing actions or just a message telling users
where to find it.

In any case, that is a minor detail, as the new patch I uploaded does
nothing to the Docking pane and simply reworks how jEdit's docking
framework saves and loads docking layouts. Is there a chance of this being
accepted into the trunk soon?

----------------------------------------------------------------------

Comment By: Shlomy Reinstein (shlomy)
Date: 2010-03-16 15:14

Message:
I don't think there's a reason to add buttons for that. Having these
functions as jEdit actions, users can bind keyboard shortcuts or toolbar
buttons for them using the Global Options dialog whenever they like.

----------------------------------------------------------------------

Comment By: Damien (kog13)
Date: 2010-03-14 18:58

Message:
I completely re-worked the patch in order to ensure consistent behavior and
to use a better XML layout. I'm not going to bother adding any buttons for
saving/loading layouts to the general options window, although I may in the
future, as the menu items are kind of tucked away and are relatively
difficult to find. The customizable docking system of jEdit is one of the
things I like most about it, so why not show it off?

I can commit the patch directly if everyone is fine with it, but I'm not
sure how big of a change this patch will be seen as. It doesn't change how
the docking framework works, but it does completely re-work how it loads
and saves layouts. Any feedback is welcome.

----------------------------------------------------------------------

Comment By: Shlomy Reinstein (shlomy)
Date: 2010-03-07 06:31

Message:
Unfortunately I will not have time soon to review your patch. Here's some
feedback based on the description only:
1. There is already support for mode-specific docking layout, which can be
configured using Global Options -> Docking. Please make sure that it works
fine with your patch, i.e. the entire docking layout is saved / restored.
2. There are also existing actions for saving / loading docking layouts
using the View -> Docking menu. Make sure these work too, and that your
patch doesn't duplicate these actions.
3. Since you're changing the format of this file, why not make it real XML
instead of such strings that require additional parsing?
Note that whether you keep it your way or change it to be real XML,
there's a need to support backward-compatibility with existing saved
layouts, where there are no square brackets.
4. About the location of the docking layouts a level deeper - no problem
from my side, except that the backward-compatibility should be maintained.



----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=300588&aid=2964432&group_id=588