[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Edge Wrapping



On Tue, Dec 11, 2012 at 08:24:00AM -0800, Tyler Thomas Hart wrote:
> > 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?

I don't like gnu getopt for that, because it uses global variables,
which is unclean and may break if we nest the parsing of two commands
into each other. We can either use a glib-getopt (I don't know if it has
the same disadvantage like gnu getopt) or a own implementation optimized
to hlwm-needs.

> 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?

There are already functions like frame*neighbour(), so the abstraction
that is needed is a hlwm-getopt.

> 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?

In the focus_edge()-case I prefer faking argc/argv. For other commands,
where there is more than just a loop around the other command, I
prefer the extra function.

> 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?

In that context, saving code or beautifying code is much more important
then the memory usage.

> + frame_foo_command()s are just split and reordered, so the diff sees it as
> adds+deletes.

OK, sorry, then I looked too shortly at the code.

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

OK good. I'd like you to wait until the next release 0.5. Then we can
refactor the code.

Regards,
Thorsten