Newsboat: When sorting the feedslist, Newsboat automatically switches to the newest `FormAction`

[dennisschagt]

Newsboat version: commit d44970b584eaa43cfd119feba8ed8f3cf940a3c9 (HEAD -> master)

Config file (copy from ~/.newsboat/config or ~/.config/newsboat/config):

# empty

Steps to reproduce the issue:

1. ENTER, ENTER (open an article view)

2. v (open the dialogs view)

3. ENTER (switch to feedlist view)

4. g, n (sort by (n)one)

Expected: Feeds are sorted and the feedlist is still being shown.

Actual: Feeds are sorted but now the article view is shown instead.

Other info you think is relevant:
To select a sort option, FeedListFormAction calls View::confirm(), which makes some strange changes to the formaction_stack:
https://github.com/newsboat/newsboat/blob/d44970b584eaa43cfd119feba8ed8f3cf940a3c9/src/view.cpp#L597-L634

[Minoru]

To select a sort option, FeedListFormAction calls View::confirm(), which makes some strange changes to the formaction_stack:

Strange indeed! We do something similar when spawning an external pager, or just a browser: we push an empty formaction onto the stack, call Stfl::reset(), then run the pager/browser, and then pop the empty formaction off the stack. Empty formacton combined with Stfl::reset effectively prevent Newsboat from drawing anything. But in this case, we don't call Stfl::reset, so what's the point?

As far as I can tell, the purpose of this is to prevent other threads from overwriting the message. ReloadThread (through Reloader) updates the "msg" of the current FormAction. Empty FormAction nullify those updates because it doesn't have an "msg" field. Meanwhile, the main thread manipulates the now-previous FormAction to display the prompt.

The above means that #1095 isn't a good solution for this issue, since it lets Reloader overwrite the message:

1. add a few dozen URLs to the urls file — enough for Newsboat to take like 5 seconds to reload them all;
2. start Newsboat with that urls file;
3. press R to start reloading everything;
4. press g to show the sorting prompt. Don't select anything!
5. after a few seconds, the prompt will be overwritten by the "Loading ..." message, which is not the expected behaviour—the prompt should stay visible until the user either picks an option or dismisses the prompt.

I believe a better way to fix this is to add a parent to the empty formaction (FormAction::set_parent_formaction()). That would make View::pop_current_formaction() return to the parent rather than the feedlist. Might be a good opportunity to make View use its own method push_empty_formaction(), too. But I didn't test this, so please take this with a grain of salt :)

[dennisschagt]

Thanks, that clears it up.

I believe a better way to fix this is to add a parent to the empty formaction (FormAction::set_parent_formaction()). That would make View::pop_current_formaction() return to the parent rather than the feedlist. Might be a good opportunity to make View use its own method push_empty_formaction(), too. But I didn't test this, so please take this with a grain of salt :)

It looks like it is not just an empty FormAction, it actually is an empty shared_ptr (nullptr).
The reason it works, seems to be that View::set_status() does not overwrite the message if the current FormAction is NULL:
https://github.com/newsboat/newsboat/blob/974207ac2b4b680a71ec808b67e6e90773089e9f/src/view.cpp#L107-L112

[Minoru]

It looks like it is not just an empty FormAction, it actually is an empty shared_ptr (nullptr).

Oh. You're exactly right ._. Looks like I've been misreading and misunderstanding that code for years!

Also, yesterday I wrote:

We do something similar when spawning an external pager …

but it's only now that I realize: this means we have the same bug in all those other places. For example, you can open an article, press u to open the URLs view, then ^G to go back to articlelist, then o to open the article in the browser—and you'll find yourself in the URLs view even though you should've remained in the article view. And you can't just copy-paste the code from #1098 to fix this, because this code lives outside View and doesn't has access to current_formaction index.

It seems to me that this should be fixed in a more systematic way:

1. extract formaction stack (vector, index, methods that manipulate it) into a class. View should hold an instance of that, and probably provide access to it;
2. round out the interface of the new class. Some View methods take formaction index as an argument, but there is no way to find out the index of the current formaction. I didn't dig into that, but "user" code probably remembers the position same way as #1098 does. That's indicative of a leaky abstraction, and is better be fixed;
3. take an opportunity to cover the new class with tests ;)
4. fix this bug either by relying on parent_formaction thing I mentioned earlier, or via a separate interface that temporarily makes the stack return nullptr instead of current formaction. Looks like it can be an add-on of a kind, i.e. instead of actually modifying the vector and index, it can just flip a bool that makes get_current_formaction return nullptr.

I realize that I'm asking for a lot of work here. If you aren't up to do any of that, I'll merge #1098 and move my suggestions into a separate issue.

[dennisschagt]

I realize that I'm asking for a lot of work here. If you aren't up to do any of that, I'll merge #1098 and move my suggestions into a separate issue.

I'm planning to look into your suggestion but I probably won't have time until next week.