Created attachment 3360 [details]
Patch to add/remove desktops at given indices

Please review this patch for merging. I have changed the API for functions screen_remove_desktop() and screen_add_desktop() to support this functionality (see commit message).

COMMIT_EDITMSG:
"""
Add functionality to add/remove workspaces at given indices.
Example usage in rc.xml:
...
    <keybind key="C-4">
      <action name="AddDesktop">
        <where>4</where>
      </action>
    </keybind>
...
will add a desktop in position 4 (one-indexed), shifting windows from 
higher desktop indices up one desktop. If there are less than 3 
desktops currently, the in between desktops will also be created.
...
    <keybind key="C-S-3">
      <action name="RemoveDesktop">
        <where>3</where>
      </action>
    </keybind>
...
will remove desktop at position 3 (one-indexed), moving its windows to 
desktop 4. If there are exactly 3 desktops, the windows are moved to 
desktop 2. If there are less than 3 desktops, nothing happens.

API changes:
screen.c:
screen_add_desktop(gboolean current); // is now
screen_add_desktop(guint index);

screen_remove_desktop(gboolean current); // is now
screen_remove_desktop(guint index);

This should be fully backwards compatible with previous scheme due to the 
changes made to openbox/actions/addremovedesktop.c
"""

Remove desktop functionality. I also would be motivated to code a more general solution for workspace management than this. I plan on posting to the mailing list asking for input.

The general workspace management idea sounds great. I am pretty sure that the added functionality of being able to add/remove desktops by index will also help a lot. 
This still leaves room for adding a basic system tray and/or window/task manager though. The standard version that installs with both the older 2.6 kernels and the current 3.x ones doesn't have any of this stuff which should be pretty basic for a desktop environment these days.

Created attachment 3365 [details]
Patch to add/remove desktops at given indices

Working patch, requires testing. See commit log for functionality specification.

I can test the patch

(In reply to comment #5)
> I can test the patch

Sweet, if you don't know what to do:
download the patch (add_remove_desktop_indices.patch)

#apply the patch against latest release
git clone git://git.openbox.org/dana/openbox openbox
cd openbox
git checkout release-3.5.0
git am ~/Downloads/add_remove_desktop_indices.patch

#build & install
./bootstrap
./configure
make
sudo make install

#Edit ~/.config/openbox/rc.xml to add some bindings to test out e.g.
    <keybind key="C-3">
      <action name="AddDesktop">
        <where>3</where>
      </action>
    </keybind>

    <keybind key="C-S-1">
      <action name="RemoveDesktop">
        <where>1</where>
      </action>
    </keybind>

#Then openbox restart might do the trick
openbox --restart
#or restart your login manager

And confirm these cases:
(1) <where>first</where>, <where>current</where>, <where>last</where> work as expected (as before) 

(2)
    <keybind key="C-3">
      <action name="AddDesktop">
        <where>5</where>
      </action>
    </keybind>
when there are 3 or less desktops results in new empty desktops in positions 4 and 5

(3)
    <keybind key="C-3">
      <action name="AddDesktop">
        <where>3</where>
      </action>
    </keybind>
when there are 5 desktops results in a new empty desktop at position 3, with windows relocated from previous desktops 3,4,5 to current desktops 4,5,6

(4)
    <keybind key="C-3">
      <action name="RemoveDesktop">
        <where>3</where>
      </action>
    </keybind>
removes one desktop from the lineup, merging 4 into 3, relocating 5->4, 6->5 etc.

(4.5)
if you are on the desktop you remove, the windows merging from the other desktop should go under your current windows

(5)
    <keybind key="C-3">
      <action name="RemoveDesktop">
        <where>5</where>
      </action>
    </keybind>
when there are 4 or less desktops does nothing

See pull request at github: https://github.com/danakj/openbox/pull/3

Not sure why resolved, see no comment saying anything.

I'm just getting tired of Bugzilla reminding me that it's open when I finished this patch a month ago.

I thought "Resolved - Works for me" was appropriate until it passed whatever screening to get into master. Or is that status reserved for something else?

In any case, I've been using this for a month and everything works as far as I've described with no regressions.

(In reply to comment #9)
> I'm just getting tired of Bugzilla reminding me that it's open when I
> finished this patch a month ago.

Hm, I don't get those, maybe you can turn them off :)

> I thought "Resolved - Works for me" was appropriate until it passed whatever
> screening to get into master. Or is that status reserved for something else?
> 
> In any case, I've been using this for a month and everything works as far as
> I've described with no regressions.

Resolved => No more work to be done for the bug. So, we can close this when the patch is merged.

that would probably be because it was set as assigned to you.

Not sure why not keep it assigned to jamie as they are working on the CL.

Hey Jamie sorry I didn't get around to testing the patch yet. I couldn't clone the repository on my system (do I need the entire gcc toolchain to use git?) 

Anyway though, if you just send me a source tarball then I can compile and test it on my system

Cheers