yeoman / yeoman-api

MIT License
1 stars 2 forks source link

Show diffs in System Pager #4

Closed mbrandl87 closed 3 weeks ago

mbrandl87 commented 10 months ago

Hi there,

like I said in https://github.com/jhipster/generator-jhipster/issues/24026

I gave it a try and eventually come up with something :)

I changed log.colored so that it first tries to spawn the system pager via another util , falling back if this should fail to log.write like it was before.

Now the problem is however, that Inquirer's prompt module seems to interfere somehow with stdin. E.g. in "less", I have to enter line by line down to the bottom before stdin is handed over to the spawned process and less accepts commands. what ever I type in between will be visible right above the next prompt. This happens when i pipe the contents via stdin and also if I dump it into a temp file beforehand and call less with that file.

Do you know a possibility how I somehow can destroy the prompt module and its UI when we received the answer and then recreate it after we return to conflicter._ask or to adapter.prompt the next time? Maybe that gets it out of the way and releasing stdin. When I push 20k log lines (ca. 900k) into the logger directly via a test main, it immediately gives control, so thats why I suspect the prompt module.

A btw I was able to npm link @yeoman/adapter and /conflicter in my JHipster test project so after npm run build in one of those the changes were available for the next generator run.

Best Martin

PS: I think async grew me some more gray hair :D

mshima commented 10 months ago

inquirer should not block here. Maybe missing await? Can you share the code?

mbrandl87 commented 10 months ago

Hi,

you can find the code at https://github.com/mbrandl87/yeoman-api/tree/pager

in workspaces/adapter/src/testing/test.ts is the main function wich logs directly and works without hickups.

the prettier doesn't like the code at the moment, I'll have to correct that before a PR ofcourse.

Oh and congrats for reaching JHipster 8.0.0!

mshima commented 10 months ago

I think we can simplify the implementation.

This is a conflicter only feature, we should try to don’t change others projects. Changing the colored api to async, is a breaking change. If the api needs to be async, create a new api specific for conflicter.

mshima commented 10 months ago

This is what I have in mind:

class ScrollableAdapter extends TerminalAdapter {
  let content;

  write(newContent) {
    content = content + newContent;
  }

  async show() {
    const scroll = new Scrollable({ content }).print();
    console.log('Up [↑] and down [↓] arrow keys to scroll. [Q] to quit');
    emitKeypressEvents(process.stdin);
    process.stdin.setRawMode(true);

    return new Promise((resolve, reject) => {
      const keyPressListener = (str, key) => {
        if (key.name == 'up') {
          scroll.scroll(-1).print();
          process.stdout.cursorTo(0, 15);
        }
        if (key.name == 'down') {
          scroll.scroll(1).print();
          process.stdout.cursorTo(0, 15);
        }
        if (key.name == 'q') {
          process.stdin.removeListener('keypress', keyPressListener);
          // Cleanup stdin
          resolve();
        }
      }
      process.stdin.on('keypress', keyPressListener);
    }
  });
}
mbrandl87 commented 10 months ago
  Ah I see, I also thought about if it would be ok to make colored async, I only did that because I couldn't find another reference in the project. But I guess its used by others using the yeoman-api?

  I gave your suggestion a try, this might work. its possible to scroll more lines at a time, maybe we can determine how many lines a page contains.
  Oh and I'd opt for using other keys, e.g. space for down and b for up, or b and n or something. Otherwise people using a screen reader must switch cursor modes. E.g. with jaws there is one mode which allows to read, but this one graps the arrow keys. 

  The cleanup is still something to figure out, if I put it in the test.ts call, I have to strg+c after setting raw mode back to false on q-press, otherwise it doesn't give back stdout  and the program doesn't terminate. without raw =false, one has to kill the wohle terminal :)

  If we could call less however, this would have the advantage of its searching capabilities. But there is the stdin problem still...

  I wasn't able to thest if your solution suffers the same effect with the generator so far, npm run build complains about some missing module in node_modules/scrollable/scrollable.d.ts  
github-actions[bot] commented 1 month ago

This issue is stale because it has been open with no activity. Remove stale label or comment or this will be closed