`tldr --update` output is displayed by pager

[tranzystorekk]

When a pager is configured, e.g. less, display.use_pager is set to true, the output of tldr --update is displayed in the pager.

Info messages should be displayed as typical output, like STDERR, regardless of pager config.

[dmaahs2017]

@tranzystorek-io can you include more information about your environment and the issue. I cannot replicate this by setting export PAGER="less".

However in the code we are printing the "Successfully updated cache." message to stdout. @niklasmohrin or @dbrgn should we just change this to an eprintln!() since it is just a progress message?

Created simple fix in #171.

[tranzystorekk]

Sorry, i meant configuring use_pager in config.toml

[tranzystorekk]

@dmaahs2017 As this concerns the use_pager setting, your fix is ineffective - tldr still first checks for pager in main and creates a pipe to the pager.

EDIT: I checked out your PR and did cargo run --release -- -u to be sure, and yes, less was run with no content.

[niklasmohrin]

@tranzystorek-io You are right, the problem is that we spawn the pager process "too early". I think this part of the code could just be moved further down. Still, what @dmaahs2017 proposes is probably a good thing to do (and also part of making this problem go away).

[tranzystorekk]

I tried moving the configure_pager block down, but the problem still remains because the cache update logic doesn't necessarily exit the process afterwards. Maybe the -u flag should be treated as a "subcommand"?

[niklasmohrin]

I was thinking that we could move the

if args.flag_pager || config.display.use_pager {
    configure_pager();
}

part into the block that only runs if a command is printed (if let Some(ref command) = args.arg_command). I have not investigated whether the cache output gets swallowed though. It does fix this particular problem.

[tranzystorekk]

Do we also want to spawn a pager when rendering local files (the -f flag)?

[niklasmohrin]

Ah, yes. I guess to stay consistent with the current behavior, the pager should be spawned in that case too. Actually, to really stay consistent, the pager should also be configured if the pages are listed. To avoid spawning it twice, maybe this block should be before the three big ifs at the end of main, something like

if (args.flag_render.is_some() || args.flag_list || args.command.is_some()) && (args.flag_pager || config.display.use_pager) {
    configure_pager();
}

Maybe it should even stay where it is right now and just be guarded with the extra condition. What do you think?

[tranzystorekk]

We need a working fix, so either of your ideas is fine. But this shouldn't mask the fact that all the logic in the main function feels stitched together. Maybe once tealdeer switches over to clap, it could be organized better.

[dbrgn]

Yeah, maybe it would make sense to finish #108 first, and then take another look at the pager logic?

I can try to give that PR another try soon.

[dbrgn]

@tranzystorek-io by the way, #108 is now merged!

[tranzystorekk]

Alright, so we can now try to rearrange/rethink the pager setup.

From the discussion so far the pager needs to be set up for rendering either cached or local page files.

The order now is:

1. Configure pager
2. Render local page and exit, if selected
3. Do cache maintenance as needed: clear, update, including automatic cache update based on update interval
4. Render requested page from cache

My proposal:

• Separate manual cache update from the automatic one
• Move local render logic near the cache render or group them together
• Configure the pager just before render

Rough order afterwards:

1. Do any manual cache actions
2. Prepare pager
3. Render local page if requested
4. Alternatively, autoupdate cache if needed and render requested cached page

[niklasmohrin]

Hm, I feel like splitting the manual and automatic updates is not the right move. I don't think there is any reason to allow for updating the online cache when rendering a local page (I might be wrong though :D), so the two update methods can stay in one place. How about something like

if render_local_page {
  configure_pager();
  render_the_page();
  exit();
}

if should_update() {
  do_the_update();
}

if should_render_from_cache {
  configure_pager();
  do_the_thing();
}

Alternatively, yesterday we had some ideas about encapsulating arguments like enable_styles and quietly into some sort of OutputManager or so. This one could also provide an interface for the main output which could then lazily spawn the pager on the first write. We are not planning on implementing this refactor before the next release though, so another approach is needed for now. What do you think?

[tranzystorekk]

Hm, I feel like splitting the manual and automatic updates is not the right move. I don't think there is any reason to allow for updating the online cache when rendering a local page

Since we provide the --update and --render flags anyway, nothing stops a user from running tldr --update --render mypage.md.

Otherwise, in my proposal the local vs cached render happens as the last step in the program (pseudocode example):

if update_flag {
    update_cache();
}

if pager_flag_or_config {
    configure_pager();
}

if render_flag() {
    render_local_file();
} else {
    if should_autoupdate() {
        update_cache();
    }

    render_cached();
}

Regarding the wrapper proposal: it seems neat but I'd assume it's not easy to design and implement as quickly as either of our initial ideas.

[dbrgn]

By the way, I removed the 1.5.0 milestone, this issue can also be fixed in a patch release. But it shouldn't be a blocker.

[tranzystorekk]

@niklasmohrin Nevermind, in my proposal the pager is still configured unconditionally, so the problem remains. Seems like we need to only configure pager conditionally and in the specific render points, like you suggested.

[niklasmohrin]

Since we provide the --update and --render flags anyway, nothing stops a user from running tldr --update --render mypage.md.

Exactly, I wonder if this is useful though. I feel like --update is its own operation and it may be better if it could only be used alone.