#322 Docking Framework Patch

None
pending-remind
Alan Ezust
None
5
2014-08-17
2010-03-05
Damien
No

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.

Discussion

1 2 3 > >> (Page 1 of 3)
  • 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.

     
  • Damien
    Damien
    2010-03-14

    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.

     
  • Damien
    Damien
    2010-03-17

    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?

     
  • Alan Ezust
    Alan Ezust
    2010-06-08

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

     
  • Alan Ezust
    Alan Ezust
    2010-06-08

    • 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.

     
  • Damien
    Damien
    2010-06-12

    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.

     
1 2 3 > >> (Page 1 of 3)