patchGNU nano - Patches: patch #9414, weeding: remove redundant...


You are not allowed to post comments on this tracker with your current authentication level.

patch #9414: weeding: remove redundant place_the_cursor() call from spotlight()

Submitted by:  David Lawrence Ramsey <dolorous>
Submitted on:  Sat 22 Jul 2017 05:57:36 AM UTC  
Priority:  5 - Normal Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open Release:  None

Tue 25 Jul 2017 07:44:02 PM UTC, comment #2:

Yes, edit_refresh() calls place_the_cursor(), but... I think edit_refresh() should not call place_the_cursor() at all. In most cases that edit_refresh() is called, either 1) the cursor is off, so it is wasteful to place the cursor, or 2) place_the_cursor() is called within ten lines, because it is necessary, or 3) immediately after refreshing the edit window, the prompt bar is redrawn and the cursor is placed there.

The edit_refresh() function seems to have been assigned three tasks: a) determine whether current is still in screen range and scroll when not, b) redraw the entire screen according to the current edittop, and c) place the cursor. I think these three tasks should be in separate functions, that are only called when needed. For example, when passing edit_refresh() to do_prompt(), all that edit_refresh() is meant to do is to redraw the edit window exactly as it was (there is no way that edittop could have changed), and there is no need to place the cursor (because it will be placed in the prompt bar).

Further, there is more redundancy in do_replace_loop(). When something needs to be interactively replaced, the first it does is call edit_refresh(). But what it should do is: scroll if needed, but if not, don't bother to refresh, because there is no need. Then, near the end of the loop, when the user chose to make a replacement (but did not choose "All"), then edit_refresh() is called when there are color strings. This seems wasteful when the next occurence of the string is way off screen. So maybe this second edit_refresh() should be replaced with a "refresh_needed = TRUE" (in case this turns out to be the last replacement made), and the first edit_refresh() in the loop can stay?

Benno Schulenberg <bens>
Project Administrator
Sat 22 Jul 2017 04:03:45 PM UTC, comment #1:

Check that. In all the places that call spotlight(), edit_refresh() is only called just before the first spotlight() call that turns the highlight on, not before the second one that turns the highlight off. So this should only work for the first call, but in actual practice, it works for both.

This is because the screen doesn't get updated after the second call, except via edit_refresh(), which redraws everything and ignores the highlight. In turn, this means that having spotlight() turn the highlight off is also unnecessary.

The updated attached patch removes the place_the_cursor() call in spotlight(), as before, but also removes the spotlight() calls that turn the highlight off. There's also a second patch included, which removes the active parameter from spotlight(); if all the function does is turn the highlight on, active is no longer used.

(file #41289, file #41290)

David Lawrence Ramsey <dolorous>
Project Administrator
Sat 22 Jul 2017 05:57:36 AM UTC, original submission:

All the places that call spotlight() call edit_refresh() just before it, and edit_refresh() calls place_the_cursor() itself. spotlight() thus places the cursor again, in the same spot that edit_refresh() just did; this is unnecessary.

The attached patch against git 4d54231 fixes this.

David Lawrence Ramsey <dolorous>
Project Administrator



Depends on the following items: None found

Items that depend on this one: None found


Carbon-Copy List
  • -email is unavailable- added by bens (Posted a comment)
  • -email is unavailable- added by dolorous (Submitted the item)

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.




    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-07-22 dolorous Attached File- => Added 0001-weeding-remove-redundant-spotlight-and-place_the_cur.patch, #41289
        Attached File- => Added 0002-weeding-remove-unused-active-parameter-from-spotligh.patch, #41290
    2017-07-22 dolorous Attached File- => Added 0001-weeding-remove-redundant-place_the_cursor-call-from-.patch, #41288

    Back to the top

    Powered by Savane 3.3