(5/27) 322 - Docking Framework Patch

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.

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.

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.

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?

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

- **assigned_to**: nobody --> shlomy

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" / ...

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.

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.

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.

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.

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.

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.

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.

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.

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?

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.

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?

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.

wm.patch (18.4Kio)

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.

in 2010 shlomy says: "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."
These actions don't work quite right for me: https://sourceforge.net/p/jedit/bugs/3798/
"Mode-specific docking layouts don't hide dockables. "
It appears that this patch addresses that issue.

- **assigned_to**: Alan Ezust

I tried to test it out today, but some of the hunks failed.
Reading the patch, my main comment is: please use Log.log() instead of System.out.println().

Damien, would you be willing to freshen up this patch for 5.2?

- **status**: open --> pending-remind