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

Add terminalutils #19

Closed
wants to merge 13 commits into from
Closed

Conversation

juancarlospaco
Copy link
Contributor

@juancarlospaco juancarlospaco commented Sep 16, 2020

  • Add prompt module, 1 proc.
  • This is the Nimble terminal "menu thingy" we are using everyday, adapted to work with stdlib.

So more than once been asked "How to import the Nimble menu thingy" or if its possible to do so,
so I tough lets make it happen, and stolen the code from Nimble, is the same code basically,
code is already there anyways..., just making it more usable.

:)

@juancarlospaco
Copy link
Contributor Author

Do you prefer other module name?, like tui or something?, just in case some day something can be added to it.
🤔

@juancarlospaco juancarlospaco changed the title Add prompt Add terminalutils Oct 16, 2020
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small corrections and simplifications (didn't test)

Co-authored-by: Danil Yarantsev <tiberiumk12@gmail.com>
juancarlospaco and others added 3 commits October 16, 2020 21:10
Co-authored-by: Danil Yarantsev <tiberiumk12@gmail.com>
Co-authored-by: Danil Yarantsev <tiberiumk12@gmail.com>
@juancarlospaco juancarlospaco marked this pull request as draft October 17, 2020 00:29
@ghost
Copy link

ghost commented Oct 17, 2020

This is the version with my small styling/functionality changes just for reference:

proc promptInteractive*(question: string, answers: openArray[string], width: Positive = 80): string =
  ## Terminal prompt that asks a `question` and returns only one of the answers from possible `answers`.
  ##
  ## .. code-block:: Nim
  ##   echo promptInteractive("is Schrödinger's Cat alive?", ["yes", "no", "maybe"])
  ##
  # Adapted from Nimble source code to stdlib, adding width optional argument.
  assert question.len > 0, "Question must not be empty"
  assert answers.len > 0, "There must be at least one possible answer"
  writeStyled(question, {styleBright})
  var
    current = 0
    selected = false
  # Incase the cursor is at the bottom of the terminal
  for arg in answers: stdout.write "\n"
  # Reset the cursor to the start of the selection prompt
  cursorUp(stdout, answers.len)
  cursorForward(stdout, width)
  hideCursor(stdout)

  # The selection loop
  while not selected:
    setForegroundColor(fgDefault)
    # Loop through the options
    for i, arg in answers:
      # Check if the option is the current
      if i == current:
        writeStyled("> " & arg & " <", {styleBright, styleUnderscore})
      else:
        writeStyled("  " & arg & "  ", {styleDim})
      # Move the cursor back to the start
      cursorBackward(stdout, arg.len + 4)
      # Move down for the next item
      cursorDown(stdout)
    # Move the cursor back up to the start of the selection prompt
    cursorUp(stdout, answers.len)
    resetAttributes(stdout)

    # Ensure that the screen is updated before input
    flushFile(stdout)
    # Begin key input
    while true:
      case getch():
      of '\t', '\x1B':
        current = (current + 1) mod answers.len
        break
      of '\r', ' ':
        selected = true
        break
      of '\3':
        showCursor(stdout)
        raise newException(OSError, "Keyboard interrupt")
      else: discard

  # Erase all lines of the selection
  for i in 0 ..< answers.len:
    eraseLine(stdout)
    cursorDown(stdout)
  # Move the cursor back up to the initial selection line
  cursorUp(stdout, answers.len)
  showCursor(stdout)
  result = answers[current]

I've found some problems with the implementation:

  1. Default width is IMO too high, it should probably be calculated as a width of the longest answer + length of the question + 2
  2. Maybe it's better to put possible answers on the next line? Then we'll have less width usage since the question could be long, but the answers will be short

@timotheecour
Copy link
Member

can this be customized to use a horizontal vs vertical layout?

@ghost
Copy link

ghost commented Oct 17, 2020

@timotheecour you mean questions on a single line vs questions on separate lines?

@timotheecour
Copy link
Member

timotheecour commented Oct 17, 2020

no I mean answers layout:

yes
no
maybe

vs

yes no maybe

@juancarlospaco juancarlospaco marked this pull request as ready for review October 19, 2020 22:52
@bitnom
Copy link

bitnom commented Oct 20, 2020

Was mentioned here

thx for efforts

Copy link
Contributor

@alaviss alaviss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this was ripped off nimble code, please insert the corresponding copyright and license for the file.

https://github.com/nim-lang/nimble/blob/master/src/nimblepkg/cli.nim#L1-L2

The license from https://github.com/nim-lang/nimble/blob/master/license.txt should be inlined into this file header and not license.txt as fusion is not dual-licensed.

Co-authored-by: alaviss <leorize+oss@disroot.org>
@juancarlospaco
Copy link
Contributor Author

Ready to merge.

@timotheecour
Copy link
Member

@juancarlospaco can you assign the review to @dom96 instead because of the code history?

@juancarlospaco
Copy link
Contributor Author

juancarlospaco commented Nov 6, 2020

@timotheecour
No; I do not have permission to do it.

@timotheecour timotheecour requested review from dom96 and removed request for timotheecour November 6, 2020 18:04
@timotheecour
Copy link
Member

done

@dom96
Copy link

dom96 commented Nov 7, 2020

So more than once been asked "How to import the Nimble menu thingy" or if its possible to do so,
so I tough lets make it happen, and stolen the code from Nimble, is the same code basically,
code is already there anyways..., just making it more usable.

There is a reason the Nimble code is available as a library, why not just use it?

@nc-x
Copy link

nc-x commented Nov 7, 2020

Firstly, I would like to say that i am one of the few people who still do not understand the reasoning behind fusion, and the criteria by which a module is supposed to be put into stdlib, or fusion, or a separate nimble package.

Personally, I would have preferred no fusion, and some of the modules from here should have been in stdlib and some as separate nimble packages, so I won't comment on whether we need this terminalutils in fusion or not.

However,

There is a reason the Nimble code is available as a library, why not just use it?

(I don't know how much code is included in the nimble as a library, so i may be wrong but anyways...)

Because nimble is not a tui library? And if I need simple tui utilities I would prefer importing a simple stdlib module, or a tui library from nimble, and not import a fullblown package manager just to use 5% of its functionality, especially considering the fact that the functionality I want to use has nothing to do with package management?

@dom96
Copy link

dom96 commented Nov 7, 2020

@nc-x nothing stops you from just importing the cli module from Nimble. Just import nimblepkg/cli and you can use it. It's a pretty lightweight module: https://github.com/nim-lang/nimble/blob/master/src/nimblepkg/cli.nim

@nc-x
Copy link

nc-x commented Nov 7, 2020

Is the cli module a separate nimble package, or does it come inside the nimble package?

the download size of nimble won't be particularly big, but still (atleast to me) it doesn't feel right to add dependency to nimble for using tui utilities.

In any case, we also need to take into account the discoverability of nimble as a tui library. Even if nimble directory shows nimble when searching for tui library, most people are going to ignore it thinking that maybe it is tagged wrong because nimble is a package manager not tui library.

@timotheecour
Copy link
Member

timotheecour commented Nov 7, 2020

it doesn't feel right to add dependency to nimble for using tui utilities

I agree. And even though nimble is, well, a package, manager, it would still be possible for it to have a require tui optional dependency (not sure if feasible at the moment, but it's doable). Libraries grow and having a dedicated tui package makes more sense than having to rely on nimble, as it'd quickly enter a friction mode (separation of concerns: nimble shouldn't focus on full blown tui). So I support having tui separately from nimble instead of requiring a dependency on nimble.

one of the few people who still do not understand the reasoning behind fusion, and the criteria by which a module is supposed to be put into stdlib, or fusion, or a separate nimble package

you are not alone; for me it's still very much an experiment, and some downsides are as follows:

  • requiring it to work with nim all the way down to 1.0.x may add too much friction
  • slower development pace compared to individual nimble package; in particular, the fact there's no "staging period" (combined with lack of semver, refs RFC: adopt semver, like almost all nimble packages #30) is a flaw; it means you can't have trial/error phase that is essential for maturing libraries, especially in between point releases; at least nim allows breaking change in devel (1.(2x+1).y); for fusion there is currently no such process laid out
  • the fact that fusion is be bundled with nim (even copied into std during install) means you'll end up with duplicate fusion imports: one from the bundled fusion, and one from pkg/fusion; this in particular will lead to duplicate symbol link errors as soon as you have some exportc symbols defined; see the whole thread here: instead of copyDir(fustion, stdlib/lib), update --path #25 (comment) that worries me a lot

on the flip side, the positive aspects about fusion would be:

  • community maintained (no package-gone-rogue/unmaintained)
  • batteries included extended standard library, where things work well together
  • common infrastructure to reduce overhead (testing/docs/etc) + more visibility

@dom96
Copy link

dom96 commented Nov 7, 2020

Is the cli module a separate nimble package, or does it come inside the nimble package?

Yeah, it's part of the nimble package. To be fair, now that Nimble supports multiple packages in the same repo, exposing the cli module as another package should be very trivial. I would prefer to see that as a PR rather than this :)

@juancarlospaco juancarlospaco deleted the prompt branch November 20, 2020 13:19
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

Successfully merging this pull request may close these issues.

6 participants