Nicholas Marriott wrote:
> Hi
>
> Thanks for the diff,
>
> I think I'd prefer to see a change to add a vi-style next-word now and the
> nonspace stuff separately, if that is possible. If it is going to be too
> time-consuming don't worry.
I'll try and get you that tonight (PST).
>> Index: mode-key.c
>> ===================================================================
>> --- mode-key.c.orig
>> +++ mode-key.c
>> @@ -83,12 +83,16 @@
>> { MODEKEYCOPY_COPYSELECTION, "copy-selection" },
>> { MODEKEYCOPY_DOWN, "cursor-down" },
>> { MODEKEYCOPY_ENDOFLINE, "end-of-line" },
>> + { MODEKEYCOPY_ENDOFNONSPACEWORD, "end-of-nonspace-word" },
>
> I don't like the use of "nonspace" in these commands, but I haven't thought of
> better names so far.
Vim uses "word" to refer to normal words, and "WORD" to refer to these.
But "end-of-WORD" struck me as even uglier than "end-of-nonspace-word".
Anyway, if you think of better names, lemme know
<snip>
>> +enum word_search_mode {
>> + WORD_SEARCH_START_OF_WORD,
>> + WORD_SEARCH_END_OF_WORD,
>> +};
>
> I'd make this something like enum window_copy_search_mode,
> WINDOW_COPY_WORDSTART, WINDOW_COPY_WORDEND.
OOC, why do you bother with such namespaces in locally-scoped definitions?
>> +
>> struct window_copy_mode_data {
>> struct screen screen;
>>
>> @@ -108,6 +113,8 @@
>> char *searchstr;
>> };
>>
>> +const char *window_copy_word_delims = " -_@";
>
> I think this is still only used in one function so it can remain local,
> although it could be static in there.
Ok.
<snip>
>> - if (skip) {
>> - /* Currently skipping non-space (until space). */
>> - if (window_copy_is_space(wp, px, py))
>> - break;
>> - } else {
>> - /* Currently skipping space (until non-space). */
>> - if (!window_copy_is_space(wp, px, py))
>> - skip = 1;
>> + is_space = window_copy_strchr(wp, px, py, nonword);
>> + if ((is_space && search_for_nonword && stop_at_end)
>> + || (!is_space && !search_for_nonword && !stop_at_end)) {
>> + /* If we've reached the spot we're looking for, we're
>> + * done looking. Yay tautologies! :) */
>> + break;
>> + } else if (is_space == search_for_nonword) {
>> + /* If we're not stopping yet, but we've just
>> + * reached a word-boundary, then toggle
>> + * search_for_nonword to indicate that. */
>> + search_for_nonword = !search_for_nonword;
>> }
>
> I wonder if there is a way to do this that is easier to read than adding extra
> flags, although the current code isn't perfect. We got rid of the skip flag in
> window_copy_cursor_previous_word maybe it isn't still necessary in here.
Well, I just renamed "skip" (whose name confused the hell outta me) to
"search_for_nonword", which gave a better indication of what it is we're
skipping.
My first go at this logic was closer to the original if-statement, but
one-level deeper:
if (window_copy_is_space(...)) {
if (search_for_nonword) {
if (stop_at_end) {
break;
} else {
search_for_nonword = 1;
}
}
} else {
if (!search_for_nonword) {
if (stop_at_end) {
search_for_nonword = 0;
} else {
break;
}
}
}
(You may find some bugs in the above; it's a rewrite from the top of my
head for what I think it looked like, and I didn't think as hard about
it this time as I did originally). I found the above fairly hard to
follow, whereas the new bit is fairly easy to think about: if "the thing
we're on" is the same as "the thing we're looking for" and "we want to
stop at that thing", then we're done. Otherwise, if "the thing we're on"
is "the thing we're looking for" (but we don't want to stop when we get
there), then we want to toggle "the thing we're looking for" instead.
Since the first two comparisons are the same in both those tests, I
guess it could also be rewritten like:
if (is_space == search_for_nonword) {
if (search_for_nonword == stop_at_end) {
break;
} else {
search_for_nonword = 0;
}
}
("search_for_nonword == stop_at_end" makes little intuitive sense;
perhaps if we renamed "stop_at_end" to "stop_at_nonword", and likewise
"is_space" -> "is_nonword", it'd be clearer?)
if (is_nonword == search_for_nonword) {
if (search_for_nonword == stop_at_end) {
...
One disadvantage to the above is that it takes advantage of the fact
that "true" for each of the vars involved is exactly 1, not "nonzero".
Which is (currently) guaranteed. If we're nervous about that, we could
wrap each in an IS_TRUE(x) macro defined as "!!(x)"...
> If you can break the diff into two I'll have a closer look at this.
<snip>
Past the patch, into the notes:
>> The window_copy_is_space() function has been replaced by
>> window_copy_strchr(), which is exactly the same except that instead of
>> hardcoding a value for what "spaces" are (several of which could not
>> really be called "spaces"), it takes a string parameter and simply
>> returns whether the designated cell matches any of the characters
>> supplied. " " is used for the "nonspace" variants, " -_@" otherwise.
>
> Not wild about the name, since it isn't doing the same thing as strchr, it
> only
> checks one cell.
Well, so does strchr only check one char. But if you find it confusing,
even briefly, than it's confusing.
How about "window_copy_char_in_set"?
-mjc
------------------------------------------------------------------------------
The Planet: dedicated and managed hosting, cloud storage, colocation
Stay online with enterprise data centers and the best network in the business
Choose flexible plans and management services without long-term contracts
Personal 24x7 support from experience hosting pros just a phone call away.
http://p.sf.net/sfu/theplanet-com
_______________________________________________
tmux-users mailing list
tmux-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tmux-users
