Re: [PATCH] Edge Wrapping

On Tue, Dec 11, 2012 at 8:24 AM, Tyler Thomas Hart <tylerthomashart _at_ gmail _dot_ com> wrote:

>2) _edge commands don't take '-i'|'-e' currently. This also means (focus l || focus_edge r) only follows
>the setting, not the flag, if '-i'|'-e' is given explicitly. This can be fixed in the current implimentation

>though, which I will do.
>Note that foo_edge with -i or -e will give different behavior, which is why I added an argument. This is
>also why I made -i|-e|-w only temporary: -w assumed -i.

Looks like I was wrong. shift_edge DOES take -i|-e. Ill submit a patch to update the docs.

>Bearing 3&4 in mind, Ill look into fixing the issue with invert_direction returning a char*, use a more

>curopts-approach (can I pull in gnu getopt?), then stash my changes until if/when they are needed in
>the future?

Also, Thorsten, questions for my just-in-case stash. I implimented HSMovement struct so that parse_movement() can be a seperate function since its called by both frame_focus_command() and frame_move_window_command(). Is there an even cleaner way to do this that I'm forgetting?

...
Side note: I did seperate frame_focus() from frame_focus_command(), since focus_edge() calling frame_focus() directly is what makes it safe. It will also make adding more commands that use frame_focus & frame_move_window easier because we don't have to 'fake' argc and argv. Is this a proper approach or is faking arg[] more elegant in your opinion?

...
Side note: besides functions invert_direction(), not too many lines are added:
+ HSMovement struct and parse_movement replaces the individual parsing in both frame_foo_command()s, so it may end up saving code (but not memory). Which do you find a priority?

+ frame_foo_command()s are just split and reordered, so the diff sees it as adds+deletes.
...
Side note: I feel like I missed some prototypes in the header.
...
Again, I'm not planning on submitting the above changes to master, for now, I just want to sit on a 'future-safe' implimentation in case something breaks/is-added in the future.

Tylo

On Tue, Dec 11, 2012 at 7:40 AM, Tyler Thomas Hart <tylerthomashart _at_ gmail _dot_ com> wrote:

Thorsten,

Thanks! No worries about the work. I need to get more familiar with the source anyways.

There were three additional improvements I sought, but only two of them is currently applicable, so I mostly agree with you.

1) The setting default_edge_wrap might be easier and more intuitive for some users. Not entirely necessary though because the 'or' method. Maybe we should write down "one-liner" methods like these in a file? In ./scripts? In a new directory?

2) _edge commands don't take '-i'|'-e' currently. This also means (focus l || focus_edge r) only follows the setting, not the flag, if '-i'|'-e' is given explicitly. This can be fixed in the current implimentation though, which I will do.

Note that foo_edge with -i or -e will give different behavior, which is why I added an argument. This is also why I made -i|-e|-w only temporary: -w assumed -i.

3) By making -'w' an argument, if more arguments are ever added in the future, or if changes to shift and focus functions ever break their return value, this keeps foo_edge commands safe from infinite loops. Note, this currently has no relevance, but it may in the future.

1 is not entirely necessary, 2 can be fixed in the current implimenation, and 3 is currently irrelevant.

So yes, I will submit focus_edge and a fix for shift_edge (allowing '-i' and '-e'), to make the changes less code, for now--

4) Also, note, my parse opts DOES allow neglecting of flags and -e|-i and -b|-w can be in either order if both are specified, the only thing is a single flag with multiple opts eg. '-ew' wasn't impliemented.

Bearing 3&4 in mind, Ill look into fixing the issue with invert_direction returning a char*, use a more curopts-approach (can I pull in gnu getopt?), then stash my changes until if/when they are needed in the future?

Thanks,
Tylo

On Tue, Dec 11, 2012 at 3:41 AM, Thorsten Wißmann <edu _at_ thorsten _minus_ wissmann _dot_ de> wrote:

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