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

Panelization is not kept while switching panel listing mode #3810

Closed
mc-butler opened this issue Apr 18, 2017 · 19 comments
Closed

Panelization is not kept while switching panel listing mode #3810

mc-butler opened this issue Apr 18, 2017 · 19 comments
Assignees
@aborodin
Labels
area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress
Milestone
4.8.20

Comments

@mc-butler
Copy link

Important

This issue was migrated from Trac:

Origin https://midnight-commander.org/ticket/3810
Reporter andrew_b (@aborodin)

How to reproduce.

  1. Make some search via 'Find File' or exec some command via 'External panelize'.
  2. Press the 'Panelize' button.
  3. Press Alt-t to switch to the another listing mode.

Result: switch from panelization to normal file listing mode.
Expected result: keep panelization result.
@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Apr 18, 2017 at 6:55 UTC (comment 1)

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

Branch: 3810_keep_panelization
[bf72382dc7bdbcc8ba1f19ad74b26aea65a33bba]

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Apr 19, 2017 at 2:20 UTC (comment 2)

  1. It's an opportunity to clean up switch_to_listing():
    1. A comment should be added to the "else" section explaining why we're doing this. A programmer reading the code might think "Panelization has to be special. If we don't reset it here, segfault will occur".
    2. We probably want to reset the filter ("*.png") too. There's no point in reseting only the panelization.
  1. set_basic_panel_listing_to() probably doesn't need to call switch_to_listing(). If it does: the "p = ..." line should come after that call.
  1. I agree with you that the "Listing mode..." dialog too shouldn't reset the panelization.

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Apr 19, 2017 at 5:47 UTC (comment 2.3)

Replying to mooffie:

[...] might think "Panelization has to be special. If we don't reset it here, segfault will occur".


(I think my English isn't clear here. It's not my native tongue, sorry. I meant: "must be special".)

A comment should be added to the "else" section explaining why we're doing this.


I was thinking along something of "/* Clear a few properties so that unaware users aren't confused by non-default behavior */". However, I now wonder: will this "else" section ever get executed? It's supposed to happen (after applying the patch) only when listing_cmd() is called (via CK_PanelListing), but this, in turn, is only called when it's a Tree or QuickView that's showing and therefore a WPanel is to be created anew (except the case when a WPanel is already showing). So perhaps the whole "else" section can go.

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Apr 19, 2017 at 7:39 UTC (comment 4)

Let me sum up my comments (as I get the feeling I added too much noise):

Unless you want listing_cmd() to reset the panelization when a listing is already shown, you just need to delete the "else" section in switch_to_listing(). (That's comment:3)

Otherwise your patch is basically fine. (That's comment:2)

(BTW, set_basic_panel_listing_to() is used only once. You may want to get rid of it. I started to write a patch to do this but I gave up because matters of style are highly subjective and I figured you'd prefer your own style.)

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Apr 19, 2017 at 8:24 UTC (comment 4.5)

  • Branch state changed from on review to on rework

Replying to mooffie:

I started to write a patch to do this

I'm working on this branch right now.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Apr 19, 2017 at 8:52 UTC (comment 6)

  • Branch state changed from on rework to on review

Done.

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Apr 19, 2017 at 17:42 UTC (comment 7)

Nice! Some comments:

  1. I think the command "ListingSwitch" should be renamed to "CycleListingMode". In the interface (and help file) we call it "listing mode", so we better stick to this. And "cycle" is a rather established term in both UIs and function names. For consistency, panel_listing_switch() could be renamed to "panel_cycle_list_type()" or similar.

(It's true that "CycleListingMode" doesn't quite parallel "PanelListingChange". Maybe we should rename "PanelListingChange" to "PanelChangeListingMode" to solve this?)

  1. doc/keybind-migration.txt needs to be updated.
  1. A reminder: NEWS should mention the command renaming.
  1. As for listing_cmd():
    1. The comment "reset panelization and back to normal listing mode" could be changed to "In case a listing (that is, a WPanel) was already showing, we reset some of its properties to their normal values."
    2. The line "set_panel_filter_to (p, g_strdup ("*"));" should be added there (either before or after resetting the panelization: it seems not to matter). ("*" works to reset the filter even if the user has turned off "Shell patterns" thereby switching to regexp syntax. Maybe we should modify set_panel_filter_to() to accept also NULL instead of segfaulting on it.)
    3. (Interestingly, resetting the panelization doesn't re-position the "cursor" to stand on the original file. But this minor bug existed before already.)

I'll later give the code another look, but overall it seems ok.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Apr 20, 2017 at 10:48 UTC (comment 7.8)

Replying to mooffie:

Nice! Some comments:

  1. I think the command "ListingSwitch" should be renamed to "CycleListingMode". In the interface (and help file) we call it "listing mode", so we better stick to this.

[...]

(It's true that "CycleListingMode" doesn't quite parallel "PanelListingChange". Maybe we should rename "PanelListingChange" to "PanelChangeListingMode" to solve this?)

I propose to use "format" instead of "mode".

  1. doc/keybind-migration.txt needs to be updated.

I think this file just should be removed. I hope, nobody is interested in keybinding names at 4.7.x time.

  1. A reminder: NEWS should mention the command renaming.

Sure.

  1. The comment "reset panelization and back to normal listing mode" could be changed to "In case a listing (that is, a WPanel) was already showing, we reset some of its properties to their normal values."

I think comment should be as short as possible. Sapienti sat.

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Apr 20, 2017 at 17:12 UTC (comment 8.9)

Replying to andrew_b:

  1. I think the command "ListingSwitch" should be renamed to "CycleListingMode".

I propose to use "format" instead of "mode".


That's certainly a possibility.

Pick any of the two. I lean towards "mode", because it's consistent with the interface, but I don't mind "format". It's for you to decide.

(I remeber a user asking on the mailing list if it's possible to switch between two (or more) custom user formats. This might be an argument againt "format", as it could give the impression of the functionality that user had in mind.)

  1. doc/keybind-migration.txt needs to be updated.

I think this file just should be removed. I hope, nobody is interested in keybinding names at 4.7.x time.


Right.

  1. The comment "reset panelization and back to normal listing mode" could be changed to "In case a listing (that is, a WPanel) was already showing, we reset some of its properties to their normal values."

I think comment should be as short as possible. Sapienti sat.


tldr: If you want to remove the comment completely, that's fine with me.

For the record: my problem with the current comment is not its shortness. Let's inspect it:

  • "reset panelization"

These two words describe what the code does. This "what" happens to be obvious, and so these words serve no real purpose. A programmer reading the code would like to know why we reset the penalization. This reset is only needed when switch_to_listing() was a no-op (in my words: "If a listing was already showing"; here, I shortened that phrase by 5 words already... and the "we" can be removed too ;-).

  • "and back to normal listing mode"

That's not true: the code doesn't change the listing mode.

Again: I won't mind if you remove the comment completely. I do remember, though, reading that code a couple of weeks ago (I was aware of the bug and worked on a patch) and not readily understanding its purpose. But that was with the old version; perhaps this new one is easier to figure out.

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Apr 21, 2017 at 14:36 UTC (comment 7.10)

Replying to mooffie:

  1. The line "set_panel_filter_to (p, g_strdup ("*"));" should be added there (either before or after resetting the panelization: it seems not to matter). ("*" works to reset the filter even if the user has turned off "Shell patterns" thereby switching to regexp syntax. Maybe we should modify set_panel_filter_to() to accept also NULL instead of segfaulting on it.)


In #3813 I make set_panel_filter_to() accept a NULL argument.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Apr 23, 2017 at 8:37 UTC (comment 11)

Please review again: [edb51c4].

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Apr 23, 2017 at 13:58 UTC (comment 12)

  • Votes set to mooffie

I believe we also need to rename "PanelListingChange" to "Panel{Change,Setup}Listing{Format,}". The verb should come first (as we did in "CycleListingFormat").

(My preference is for "Setup" over "Change", as the latter isn't as definite. But then one could ask why we have "OptionsXYZ" commands there instead of "SetupXYZ".)

Except for this the code looks fine to me (I tested it), so I'm voting for it.

(BTW: IMHO, the comment in "(listing_cmd): reset panel filter" should go because it doesn't explain the difficulty in the code (and there is a difficulty there). The comment just repeats what we're doing ("we reset the ...") instead of explaining why we're doing it.)

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Apr 23, 2017 at 15:44 UTC (comment 12.13)

I added two fixup commits for review.

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Apr 23, 2017 at 17:40 UTC (comment 13.14)

Replying to andrew_b:

I added two fixup commits for review.


I don't see it. You probably forgot to push.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Apr 23, 2017 at 17:45 UTC (comment 14.15)

Replying to mooffie:

You probably forgot to push.

Yes. Sorry. Done.

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Apr 23, 2017 at 18:14 UTC (comment 16)

All looks fine to me!

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Apr 24, 2017 at 13:19 UTC (comment 17)

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

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Apr 24, 2017 at 13:20 UTC (comment 18)

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

Merged to master: [ed1ed00].

git log --pretty=oneline e4983a1..ed1ed00

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Apr 24, 2017 at 13:24 UTC (comment 19)

  • Status changed from testing to closed

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
Milestone
4.8.20
Development

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