Ticket #3720 (closed defect: fixed)

Opened 7 years ago

Last modified 7 years ago

Viewer doesn't show search error messages

Reported by: mooffie Owned by: andrew_b
Priority: major Milestone: 4.8.19
Component: mcview Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

For example, when searching (F7) using an invalid regexp pattern (say "*" or "["), we aren't shown an appropriate error message telling us the pattern is bad. The editor does show such error messages.

(The only "error" message the viewer shows is "Search string not found".)

Attachments

3720-make-viewer-show-search-error-messages.patch (1.7 KB) - added by mooffie 7 years ago.

Change History

comment:1 Changed 7 years ago by mooffie

  • Owner set to mooffie
  • Status changed from new to assigned

Changed 7 years ago by mooffie

comment:2 Changed 7 years ago by mooffie

NOTE: to understand the patch, you need to read the documentation for mc_search_run()'s return value, in #3693.

comment:3 Changed 7 years ago by mooffie

Explanation for the patch:

The main problem(s) are in mcview_do_search().

mcview_do_search() has the following structure:

{
  boolean found = mcview_find();

  if (orig_search_start != 0 && !found) {
    if (query("Continue from beginning?"))
      search from beginning
  }

  if (!found
        && (view->search->error == MC_SEARCH_E_ABORT
             || view->search->error == MC_SEARCH_E_NOTFOUND))
  {
    ...
    display error message
  }
}

There are two bugs here:

(1) The test view->search->error == MC_SEARCH_E_ABORT || view->search->error == MC_SEARCH_E_NOTFOUND checks for only two of the three possibilities of the failure of mc_search_run(), as documented in the patch at #3693. This test precludes other error codes, like invalid pattern. The fix is simply to remove this test altogether.

(2) We start a "Continue from beginning?"-search even if the failure was because of pattern error or user abort. Instead, we should do this only if the failure was of the MC_SEARCH_E_NOTFOUND kind. Otherwise, besides doing the wrong thing, we also may obliterate a useful error message.

There's also a bug in the backwards search, in mcview_find():

The backwards search, in mcview_find(), works as follows:

while (we're past the beginning) {

  go backwards a bit

  bolean ok = mc_search_run()

  if (ok)
    return TRUE;

  /* Abort search. */
  if (!ok && view->search->error == MC_SEARCH_E_ABORT)
    return FALSE;
}

return FALSE;

(3) The "abort" should occur for any error other than MC_SEARCH_E_NOTFOUND, not just for interactive user abort.

(BTW, a similar bug perhaps exists in the backwards search in the editor. I'll check this out and start a new ticket if that's the case.)

Last edited 7 years ago by mooffie (previous) (diff)

comment:4 Changed 7 years ago by andrew_b

  • Owner changed from mooffie to andrew_b
  • Status changed from assigned to accepted
  • Votes for changeset set to andrew_b
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.19

Branch: 3720_mcview_search_error_messages
changeset:46f01d70b234a35c2f3f373ffd7ef7f6bce34121

comment:5 Changed 7 years ago by andrew_b

  • Branch state changed from on review to approved

comment:6 Changed 7 years ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

comment:7 Changed 7 years ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.