8000 Internal viewer's wrong position on view find file's result · Issue #3530 · MidnightCommander/mc · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Internal viewer's wrong position on view find file's result #3530

Closed
mc-butler opened this issue Sep 30, 2015 · 29 comments
Closed

Internal viewer's wrong position on view find file's result #3530

mc-butler opened this issue Sep 30, 2015 · 29 comments
Assignees
@aborodin
Labels
area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress ver: 4.8.14 Reproducible in version 4.8.14
Milestone
4.8.15

Comments

@mc-butler
Copy link

Important

This issue was migrated from Trac:

Origin https://midnight-commander.org/ticket/3530
Reporter eshkrig (eshkrig@….com)
Mentions egmont@….com (@egmontkob)

Hi!

Sorry for my English.

Since version 4.8.14 there is a problem: "Find File" -> "Content" -> "View - F3" - line is not on the top in the internal viewer if found line is near EOF. Interested line maybe anywhere on the screen - this is very inconvenient. Old good behaviour is broken.

Please, fix it.

Thank you!

Note

Original attachments:
@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Sep 30, 2015 at 18:53 UTC (comment 1)

It was me rewriting the core of the viewer for 4.8.14, in ticket #3250. This particular question, the behavior near the end of the file was mostly discussed in ticket #3256.

Both tickets had some quite heated discussions on what the behavior should be. After a while these discussions stalled without an agreement. Later on they applied my work with my design.

See those threads (especially the opening description of 3256) for why I chose this behavior.

I'd be happy to implement the other behavior as an option if there's popular demand for that and if mainstream developers express that they'd accept having such a new config option. But first I'd also like to hear your opinion on why you find the current behavior inconvenient. The current behavior is how pretty much every modern graphical application works nowadays.

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Sep 30, 2015 at 19:03 UTC (comment 2)

  • Cc set to egmont@….com

@mc-butler
Copy link
Author

Changed by mike.dld (mikedld@….com) on Sep 30, 2015 at 19:04 UTC (comment 3)

It's not about whether new scrolling behavior if better than the old one. It's about the fact that with old behavior, when you press F3 inside search dialog you're used to look at the very top of the screen to see the matched line. Now you have to look for it through all the screen, or press F7+Return once inside the viewer so that the match is highlighted. I think an improvement in the form of highlighting the match right away would be fine (at least for me, can't talk for OP).
-->
@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Sep 30, 2015 at 19:47 UTC (comment 4)

I don't understand what you mean... the search result is highlighted, isn't it? How does F7+Return help? What are the exact keys you press to search so that the result is not highlighted? Seems you may have found another bug.

@mc-butler
Copy link
Author

Changed by mike.dld (mikedld@….com) on Sep 30, 2015 at 19:57 UTC (comment 5)

The steps are:

  1. Open search dialog with Meta+?
  2. Type in the text to search, press Return to start the search
  3. In search results dialog, select the matched file, press F3

Supposing the match is at the end of file,

  • old behavior: matched line is at the top of the screen, convenient as you know where to look for it
  • new behavior: matched line is somewhere on the screen, need to look for it
  • suggested behavior: matched line is somewhere on the screen, but it is highlighted as if you just pressed F7 (to search inside the file) and Return (note, search dialog already contains search phrase entered on step 2).

I'm using 4.8.14 and right now the result is not highlighted after step 3.

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Sep 30, 2015 at 20:15 UTC (comment 6)

I see, you're right! I completely missed this use case.

Could you please file a separate bugreport for this?

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Sep 30, 2015 at 20:18 UTC (comment 7)

My bad, I didn't realize the original report was also about this, rather than just simply searching in mcview. Sorry! I guess we could then keep this ticket for highlighting the search result when going there from Find File.

eshkrig, would this also be good for you? The search result wouldn't necessarily be on the top, but would be highlighted.

@mc-butler
Copy link
Author

Changed by eshkrig (eshkrig@….com) on Oct 1, 2015 at 6:05 UTC (comment 8)

Either method is suitable: matched line is at the top of the screen {and|or} it is highlighted.

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Oct 1, 2015 at 20:22 UTC (comment 9)

Thanks for your input. I'll do the highlighting. It's more complicated than I hoped it would be, but already made some progress. I hope it'll be done in a couple of days.

@mc-butler
Copy link
Author

Changed by eshkrig (eshkrig@….com) on Oct 2, 2015 at 6:19 UTC (comment 10)

Thank you very match!

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Oct 2, 2015 at 23:21 UTC

Proof of concept fix

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Oct 2, 2015 at 23:22 UTC (comment 11)

First version attached. It seems to work, but still requires heavy cleanup.

Can you guys recompile and try mc with this patch, and send feedback?

@mc-butler
Copy link
Author

Changed by mike.dld (mikedld@….com) on Oct 3, 2015 at 15:25 UTC (comment 12)

When applied to 4.8.14, I get this crash:

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000001ce6

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   mc                            	0x0000000105fb466b str_utf8_make_make_term_form + 64
1   mc                            	0x0000000105fb3579 str_utf8_fit_to_term + 28
2   mc                            	0x0000000105f9b498 listbox_draw + 288
3   mc                            	0x0000000105f9aabc listbox_callback + 67
4   mc                            	0x0000000105f8a42a find_add_match + 262
5   mc                            	0x0000000105f89b85 do_search + 1138
6   mc                            	0x0000000105f8a4a9 find_callback + 28
7   mc                            	0x0000000105f7b45d dlg_run + 147
8   mc                            	0x0000000105f89047 find_file + 5503
9   mc                            	0x0000000105f9fce7 midnight_execute_cmd + 687
10  mc                            	0x0000000105f9f1df midnight_callback + 727
11  mc                            	0x0000000105f7b1b6 dlg_process_event + 861
12  mc                            	0x0000000105f7b4a3 dlg_run + 217
13  mc                            	0x0000000105f9edc7 do_nc + 3491
14  mc                            	0x0000000105f637c3 main + 1295
15  libdyld.dylib                 	0x00007fff89e075c9 start + 1

Thread 0 crashed with X86 Thread State (64-bit):
  rax: 0x0000000105fb355d  rbx: 0x000000010601d950  rcx: 0x0000000000000006  rdx: 0x0000000000000011
  rdi: 0x0000000000001ce6  rsi: 0xffffffffffffffff  rbp: 0x00007fff59c9d0e0  rsp: 0x00007fff59c9d0b0
   r8: 0x0000000106020a35   r9: 0x0000000000000000  r10: 0x00000000000000eb  r11: 0x0000000000000000
  r12: 0x0000000000000000  r13: 0x00007ff569e29ce0  r14: 0x0000000000001ce6  r15: 0xffffffffffffffff
  rip: 0x0000000105fb466b  rfl: 0x0000000000010286  cr2: 0x0000000000001ce6

When applied to master, I get a bit different crash:

Exception Type:        EXC_CRASH (SIGABRT)
Exception Codes:       0x0000000000000000, 0x0000000000000000

Application Specific Information:
abort() called
*** error for object 0x7f95525074f0: incorrect checksum for freed object - object was probably modified after being freed.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib        	0x00007fff92773286 __pthread_kill + 10
1   libsystem_c.dylib             	0x00007fff899ff9b3 abort + 129
2   libsystem_malloc.dylib        	0x00007fff8f96dfe2 szone_error + 625
3   libsystem_malloc.dylib        	0x00007fff8f96335d szone_free_definite_size + 3678
4   libglib-2.0.0.dylib           	0x000000010ae29552 g_string_free + 30
5   mc                            	0x000000010abf58fe mc_search__run_regex + 773
6   mc                            	0x000000010abd1937 do_search + 908
7   mc                            	0x000000010abd2341 find_callback + 28
8   mc                            	0x000000010abc325d dlg_run + 147
9   mc                            	0x000000010abcff3d find_file + 1337
10  mc                            	0x000000010abe7b9f midnight_execute_cmd + 687
11  mc                            	0x000000010abe7097 midnight_callback + 727
12  mc                            	0x000000010abc2fb6 dlg_process_event + 861
13  mc                            	0x000000010abc32a3 dlg_run + 217
14  mc                            	0x000000010abe6c7f do_nc + 3491
15  mc                            	0x000000010abab2e9 main + 1297
16  libdyld.dylib                 	0x00007fff89e075c9 start + 1

Thread 0 crashed with X86 Thread State (64-bit):
  rax: 0x0000000000000000  rbx: 0x0000000000000006  rcx: 0x00007fff55055f78  rdx: 0x0000000000000000
  rdi: 0x0000000000000513  rsi: 0x0000000000000006  rbp: 0x00007fff55055fa0  rsp: 0x00007fff55055f78
   r8: 0x00000000fffffff0   r9: 0x0000000000000010  r10: 0x0000000008000000  r11: 0x0000000000000206
  r12: 0x0000000000000001  r13: 0x000000010adb8000  r14: 0x00007fff76452300  r15: 0x000000010ac98000
  rip: 0x00007fff92773286  rfl: 0x0000000000000206  cr2: 0x00007fff76646fd8

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Oct 3, 2015 at 16:46 UTC (comment 13)

Oops...

In src/filemanager/find.c, around line 873 there's a g_malloc (sizeof location); could you please change that to g_malloc (sizeof *location); [just add one star character] and try again?

This is a stupid mistake and I wonder why it didn't crash/misbehave for me :)

@mc-butler
Copy link
Author

Changed by mike.dld (mikedld@….com) on Oct 3, 2015 at 17:29 UTC (comment 14)

That works well. The only thing missing is horizontal positioning when lines are longer than screen width and match is outside the screen (with line wrapping disabled). But since searching within the viewer with F7 doesn't scroll horizontally to the match either, I think this is another issue, don't remember whether pre 4.8.14 mc did that.

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Oct 3, 2015 at 17:35 UTC (comment 15)

Cool, I'll go ahead and clean up this patch.

I think horizontal scrolling to the match never worked, IIRC there's a ticket for that.

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Oct 3, 2015 at 18:52 UTC

Fix

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Oct 3, 2015 at 18:57 UTC (comment 16)

Please test and review the attached patch thoroughly.

I expanded the "invisible" data stored for each list entry. So far it only contained the directory in which the file resides - a perfect match for list entries' userdata field. Now they encapsulate the directory name, plus start and end offset for the matches. For this reason a complete struct needs to be allocated and its pointer passed there. I also needed to add infrastructure to optionally automatically free the userdata when the list entry is removed.

mcview_viewer() and friends also had to be extended by two new arguments: the start and end of the area to be highlighted.

The logic in find_add_match which keeps track of the offset within the file is very complicated. I hope it's bugfree (I've tested even with embedded NUL bytes) but a second eye would be great.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Oct 4, 2015 at 9:43 UTC (comment 17)

So, just to be clear about it, after this patch, it will highlight the match, but the match is not going to be at the top of the screen as before? I'm also kind of missing this, it was hugely convenient. In fact, I use 4.8.13 at home still...

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Oct 4, 2015 at 9:49 UTC (comment 18)

Not if the match is near the end of the file.

The new viewer - discussed in the aforementioned links - makes it a hard requirement never to waste screen real estate at the bottom when it could be used to display more data from the file. A file that's taller than the viewport can't be scrolled beyond the position where the end of the file appears at the bottom of the viewer. No exceptions.

Just as e.g. your browser also won't scroll the bottom of the content to the top of your screen, leaving tons of whitespace below that does not belong to the page.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Oct 4, 2015 at 14:09 UTC (comment 15.19)

Replying to egmont:

I think horizontal scrolling to the match never worked, IIRC there's a ticket for that.

#2132

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Oct 4, 2015 at 14:15 UTC (comment 16.20)

  • Milestone changed from Future Releases to 4.8.15
  • Status changed from new to accepted
  • Owner set to andrew_b
  • Branch state changed from no branch to on review

Replying to egmont:

Please test and review the attached patch thoroughly.

Branch: 3530_view_highlight_search_result
Initial [c95a4427c6ec4085cdd273313f8e9f80b414cfa7]

I've split the patch to three commits:

  1. WLEntry::free_data and related code.
  2. Viewer with externally set boundaries of search result.
  3. Highlight of search result from Find File.

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Oct 4, 2015 at 14:22 UTC (comment 21)

Awesome, thanks! I guess I should have done it :)

I was primarly wondering whether you liked the overall approach... but I'm glad that you like the patch as well.

One thing to note: in the spirit of #39 this patch introduces another bug: in Parsed mode a totally irrelevant segment of the file could be highlighted when you open it. Preventing this bug would require a totally different, more complicated approach (maybe opening the file at the given line, and then performing the search again). I honestly don't care too much and really don't want to start over this patch with a different approach, I think we could live with this minor bug and add a note to #39 to address this one day.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Oct 18, 2015 at 5:33 UTC (comment 22)

  • Branch state changed from on review to approved
  • Votes set to andrew_b

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Oct 18, 2015 at 5:34 UTC (comment 23)

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

Merged to master: [5ef1b4b5f972575ccb306afbe001354fad7cce85].

git log --pretty=oneline 546404c..5ef1b4b

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Oct 18, 2015 at 5:35 UTC (comment 24)

  • Status changed from testing to closed

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Oct 18, 2015 at 16:35 UTC (comment 25)

New merge head: [a6a7806].

git log --pretty=oneline 2cee5e5..a6a7806

@mc-butler
Copy link
Author

Changed by zaytsev-work (@zyv) on Nov 10, 2015 at 16:03 UTC (comment 26)

Hi Andrew,

https://www.midnight-commander.org/wiki/NEWS-4.8.14?action=diff&version=41

this was supposed to go into NEWS-4.8.15, right?

Thanks!

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Nov 10, 2015 at 17:27 UTC (comment 26.27)

Replying to zaytsev-work:

https://www.midnight-commander.org/wiki/NEWS-4.8.14?action=diff&version=41

this was supposed to go into NEWS-4.8.15, right?

Yes. Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aborodin aborodin

Labels
area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress ver: 4.8.14 Reproducible in version 4.8.14
Milestone
4.8.15
Development

No branches or pull requests
2 participants
@aborodin @mc-butler
0