Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show diffs in System Pager #4

Open
mbrandl87 opened this issue Nov 4, 2023 · 5 comments
Open

Show diffs in System Pager #4

mbrandl87 opened this issue Nov 4, 2023 · 5 comments

Comments

@mbrandl87
Copy link

Hi there,

like I said in
jhipster/generator-jhipster#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
Copy link
Member

mshima commented Nov 4, 2023

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

@mbrandl87
Copy link
Author

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
Copy link
Member

mshima commented Nov 5, 2023

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
Copy link
Member

mshima commented Nov 5, 2023

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
Copy link
Author

  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  

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants