Ticket #3810 (closed defect: fixed)

Opened 4 months ago

Last modified 4 months ago

Panelization is not kept while switching panel listing mode

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

Description

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.

Change History

comment:1 Changed 4 months ago by andrew_b

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

comment:2 follow-up: ↓ 3 Changed 4 months ago by mooffie

  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.

comment:3 in reply to: ↑ 2 Changed 4 months ago by mooffie

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.

comment:4 follow-up: ↓ 5 Changed 4 months ago by mooffie

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.)

Last edited 4 months ago by mooffie (previous) (diff)

comment:5 in reply to: ↑ 4 Changed 4 months ago by andrew_b

  • 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.

comment:6 Changed 4 months ago by andrew_b

  • Branch state changed from on rework to on review

Done.

comment:7 follow-ups: ↓ 8 ↓ 10 Changed 4 months ago by 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. 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.

comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 4 months ago by andrew_b

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.

comment:9 in reply to: ↑ 8 Changed 4 months ago by mooffie

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.

comment:10 in reply to: ↑ 7 Changed 4 months ago by mooffie

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.

comment:11 Changed 4 months ago by andrew_b

comment:12 follow-up: ↓ 13 Changed 4 months ago by mooffie

  • Votes for changeset 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.)

comment:13 in reply to: ↑ 12 ; follow-up: ↓ 14 Changed 4 months ago by andrew_b

I added two fixup commits for review.

comment:14 in reply to: ↑ 13 ; follow-up: ↓ 15 Changed 4 months ago by mooffie

Replying to andrew_b:

I added two fixup commits for review.


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

comment:15 in reply to: ↑ 14 Changed 4 months ago by andrew_b

Replying to mooffie:

You probably forgot to push.

Yes. Sorry. Done.

comment:16 Changed 4 months ago by mooffie

All looks fine to me!

comment:17 Changed 4 months ago by andrew_b

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

comment:18 Changed 4 months ago by andrew_b

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

Merged to master: [ed1ed00073c00ccdd94a3b7e5c471c18711ebaca].

git log --pretty=oneline e4983a1..ed1ed00

comment:19 Changed 4 months ago by andrew_b

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