Re: [PATCH] Edge Wrapping

[Thorsten Wißmann <edu _at_ thorsten _minus_ wissmann _dot_ de>]

Hi Tylo,

I appreciate the amount of work but I think most of it isn't needed as
soon only focus_edge() is added:

> On Mon, Dec 10, 2012 at 4:59 PM, Tyler Thomas Hart wrote:
> > In order to do this, I added command focus_edge which behaves just like
> > shift_edge.

That's OK.

> > I implimented [optional] edge wrapping (cyclical shifting and focusing).
> > This means, when no neighbor is found, movement goes all the way to the
> > direction opposite the DIRECTION argument.

But why do you need that? I thought it is enough to have focus,
focus_edge and the "or" command.

> > This is currently used by specifying '-w' as the argument to shift and
> > focus, assuming '-i' regardless of the setting. Hopefully, I intend to
> > change the way shift and focus parse flags, and add a setting for
> > always_wrap that way '-i'|'-e' & '-w'|'-nw??' can be used together.

Two different arguments to give options? It is more intuitive to have
one (optional) argument where the user can pass getopt-like
short-options (or maybe multiple arguments) but it should not be
enforced that there the first argument is for -i vs -e and the second is
-w vs -nw. Especially because it is inconsistent to:

    shift ['-i'|'-e'|'-w'] 'DIRECTION'

> > I also divided up frame_focus_command into two functions, frame_focus and
> > frame_focus_command. Frame_focus_command simply parses input flags and
> > checks settings, while frame_focus performs the action. Note, I did this
> > also so that focus_edge NEVER recursively calls frame_focus with wrapping,
> > so that infinite loops are ALWAYS avoided, regardless of flags or settings.
> >
> > I did the same modifications to frame_focus_window_command,
> > frame_focus_window and focus_edge.
> >
> > Let me know if I missed anything. I'll impliment always_wrap setting and
> > better flag parsing ASAP.

The separation into the command and the actual functionality is OK, if
the functionality-part is called from somewhere else (and isn't only
used by the function).

And it's unneeded (and IMO unclean) that direction_invert() returns a
char*. Also the if-strcmp-elseif cascade isn't clean (I only did it
because there were only two flags -i and -e) and leads to much code.

On Mon, Dec 10, 2012 at 10:06:34PM -0800, Tyler Thomas Hart wrote:
> I am now parsing shift and focus with a single function, which parses args
> into a struct, HSMovement. This contains the direction, flags, and calling
> name. This is done in a for loop which assumes direction is specified last.
> Let me know if this argument order thing is inconsistent with how things
> were done/will be done.

In my opinion there is no need for adding that much code if you already
can achieve the same with just a few lines in the autostart.

> I implimented the setting default_edge_wrapping and seperated '-i'|'-e'
> from '-w'|'-b'. Since '-w' stands for "wrapping," I chose '-b' to stand for
> "bound." Let me know if there are better flag letters.

Basically -b is a better flag but -w/-b is unneeded anyway (as mentioned
above).

So is there some feature I missed? If not, then please resend a patch
that just adds focus_edge (including the command completion).

Regards,
Thorsten