ratpoison-devel
[Top][All Lists]
Advanced

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

Re: [RP] select patch


From: Jeff Abrahamson
Subject: Re: [RP] select patch
Date: Mon, 23 Jun 2014 16:30:18 +0200

Funny how documentation is always the trailing bit. ;-)

This is looking very nice.  Thanks for all your work on this!

A couple (easy) comments:

Just delete the stuff you're deleting, don't comment it out.  Git remembers what was there before, and it just makes your diffs longer.  I know it's useful while you're developing.

I'm acutely aware that I'm the guy who called for the optional behavior -- but the more I think about it, the more I think the default should be the behavior you propose.  It is a very good idea.
On that subject, the name "SELECT_SIMPLE" seems prejudiced by the old behavior and otherwise devoid of meaning.  What do you think of SELECT_OK_SELECTED ?

When you define MAX_WINDOW_NAME_LENGTH, I think the value is arbitrary, just long enough to distinguish between valid and invalid strings in reasonable time.  Perhaps include a comment to that effect so that future readers don't pause to wonder on the significance of that number.

Personal tick:  Functions are most readable below ten or so lines.  By the time I can't see the whole function in my editor, I have more trouble understanding as I read.  In that light, and without remarking on other examples of such, perhaps find_window_name() could easily be broken into a helper function or two (with local linkage), such as a

compare_window_name_function window_name_matcher(size_t& compare_length).


Jeff Abrahamson
+33 6 24 40 01 57   <-- brièvement indisponible le 4 juillet
+44 7920 594 255    <-- will change 18 July

http://jeff.purple.com/
http://blog.purple.com/jeff/



On 20 June 2014 15:52, Johannes Altmanninger <address@hidden> wrote:
Thanks! Now everything works just fine.
The only thing that is missing now is the documentation :)


On 06/20/2014 03:14 PM, Peter Pentchev wrote:
On Fri, Jun 20, 2014 at 02:43:48PM +0200, Johannes Altmanninger wrote:
Now I have rewritten find_window_name() again, it is much cleaner this way,
no need for unnecessary temporary variables anymore :)
It works the same as before but I still get the "assignment from
incompatible pointer type" warning...
Take a look at how strncmp() is declared on your system; it is most
probably something like

   int strncmp(const char *s1, const char *s2, size_t len);

Your typedef should be the same.

G'luck,
Peter



reply via email to

[Prev in Thread] Current Thread [Next in Thread]