Panelization is not kept while switching panel listing mode

[andrew_b]

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.

[andrew_b]

Branch: 3810_keep_panelization
changeset:bf72382dc7bdbcc8ba1f19ad74b26aea65a33bba

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

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

3. I agree with you that the "Listing mode..." dialog too shouldn't reset the panelization.

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

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

[andrew_b]

Replying to mooffie:

I started to write a patch to do this

I'm working on this branch right now.

[andrew_b]

Done.

[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?)

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

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

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

[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".

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

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

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

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

[mooffie]

Replying to mooffie:

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

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

[andrew_b]

Please review again: [edb51c445783af6188eb272761ca364d9043dda0].

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

[andrew_b]

I added two fixup commits for review.

[mooffie]

Replying to andrew_b:

I added two fixup commits for review.

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

[andrew_b]

Replying to mooffie:

You probably forgot to push.

Yes. Sorry. Done.

[mooffie]

All looks fine to me!

[andrew_b]

Merged to master: [ed1ed00073c00ccdd94a3b7e5c471c18711ebaca].

git log --pretty=oneline e4983a1..ed1ed00