Skip to content

Conversation

@zlatanvasovic
Copy link
Contributor

@zlatanvasovic zlatanvasovic commented Apr 29, 2020

Closes #79.

Summary of the changes:

  • page format greatly improved
  • text properly colored and highlighted
  • blank color removed
  • cprint replaced with colored
  • get terminal size functions removed

Screenshot:
Screenshot from 2020-04-30 21-58-44

@zlatanvasovic
Copy link
Contributor Author

@MasterOdin What do you think, should this be merged before #96? I want to avoid fixing tests twice at any cost.

@MasterOdin
Copy link
Contributor

I'm fine updating my PR after this gets merged.

@zlatanvasovic
Copy link
Contributor Author

ping @waldyrious

@zlatanvasovic zlatanvasovic force-pushed the newline branch 2 times, most recently from be70a51 to 09e4209 Compare April 30, 2020 08:57
@zlatanvasovic
Copy link
Contributor Author

zlatanvasovic commented Apr 30, 2020

@felixonmars Why was Manually adding painted in blank spaces added in the first place? It just slows down the program and also adds unnecessary whitespace on copy/paste.

I removed it in this PR.

@felixonmars
Copy link
Collaborator

git blame shows that it was added in #20

@zlatanvasovic
Copy link
Contributor Author

zlatanvasovic commented Apr 30, 2020

Pretty much perfect now.

Screenshot from 2020-04-30 17-54-09

Maybe it's even better with 2 indents for title and description?
Screenshot from 2020-04-30 17-57-06

@MasterOdin
Copy link
Contributor

MasterOdin commented Apr 30, 2020

It feels like you're missing the point of the BLANK color, and removing it is not right, as it would prevent the styling of background for an entire sheet if so desired (like in the screenshot from #20).

command: TLDR_COLOR_BLANK="white on_blue" TLDR_COLOR_NAME="white on_blue" TLDR_COLOR_DESCRIPTION="white on_blue" TLDR_COLOR_EXAMPLE="green on_blue" python3 tldr.py cd

Before:
Screenshot 2020-04-30 17 48 08

After:
Screenshot 2020-04-30 17 48 37

Note, this does require my fix from #96 or commenting out lines 223-225 to get the color + attribute to work properly on your branch.

@zlatanvasovic
Copy link
Contributor Author

zlatanvasovic commented Apr 30, 2020

Compare it to the way git (or pretty much any standard console tool) does it (all is highlighted, for the record, as my terminal background is white):
Screenshot from 2020-04-30 18-52-49
Only colored parts are supposed to be selected. Adding spaces and blank backgrounds complicates the display and copy/paste.

What you seem to miss here is that your PR solves the problem you described:
Screenshot from 2020-04-30 18-56-28
By setting the colors to default, highlighting matches perfectly.

@zlatanvasovic
Copy link
Contributor Author

@MasterOdin For comparision, this is how it looks after merging your PR:
Screenshot from 2020-04-30 18-59-53

@MasterOdin
Copy link
Contributor

MasterOdin commented Apr 30, 2020

I would say set TLDR_COLOR_BLANK to the empty string ("") by default, and that would fix your problem?

I mean, feel free to remove TLDR_COLOR_BLANK, but the idea of setting a background was something someone wanted when they added it.

Only colored parts are supposed to be selected. Adding spaces and blank backgrounds complicates the display and copy/paste.

I can buy complicating writing the display logic, but not sure I buy copy/paste. Testing out master in a terminal window on cmd.exe, macOS iTerm2 using zsh, and Ubuntu bash, when I selected and copied the entire page, there was no extraneous spaces on each line, regardless of background color or not. I suppose you could just skip printing out the content if TLDR_COLOR_BLANK is empty if it really messes up copy/paste for you, but it does sound like a bug in your terminal?

@zlatanvasovic
Copy link
Contributor Author

zlatanvasovic commented Apr 30, 2020

@MasterOdin This is what I get in VSCode after copying and pasting the output from python3 tldr.py tldr. First screenshot is using Copy, the second is using Copy as HTML.

  1. master
    Screenshot from 2020-04-30 19-36-11
    Screenshot from 2020-04-30 19-38-12

  2. newline
    Screenshot from 2020-04-30 19-38-34
    Screenshot from 2020-04-30 19-38-51

(Note: it may seem there is trailing whitespace at the end of all lines, but that's not the case, just the peculiarity of VSCode's highlighting)

Bonus:
HTML display also confirms that formatting on newline is correct (as it is on master):
Screenshot from 2020-04-30 19-41-19


To conclude, with all the changes I proposed (proper highlighting and removing the whitespace), blank as a color seems redundant completely.

I also don't see the reason to set tldr's background to something special (as basically no other command-line tool does that). The biggest problem I see with setting the background is that it requires adding whitespace. Such approach also bugs multi-line display of a single line, which is very common on 80x24 terminals.

@zlatanvasovic
Copy link
Contributor Author

zlatanvasovic commented Apr 30, 2020

Note: lines 37-74 can be removed since columns and rows size of the terminal is not used anymore. Rows were actually never used, while columns were used only to add spaces.

@MasterOdin
Copy link
Contributor

The background stuff was done in #1 which I guess was because the node client did at the time as well, and has just been carried forward since then. I cannot say whether or not people have been using this option, just that it has existed in tldr for 6 years at this point.

I also don't see the reason to set tldr's background to something special (as basically no other command-line tool does that).

That's the trick right? It's hard to say what end users are doing. While I also don't personally bother setting the background color, that's not to say there's someone out there who isn't. Given that it was added as a set hard-coded option to be a on_blue background, it's clear there's at least one person who thought that was the ideal way to display the pages in a terminal.

@zlatanvasovic
Copy link
Contributor Author

zlatanvasovic commented Apr 30, 2020

I just checked tldr-node-client and it doesn't support custom background. This is how node client highlights the text (same as I proposed here, minus the correct highlighting for the - Example descriptions):
Screenshot from 2020-04-30 21-04-54
(it also doesn't add any trailing whitespace...)

While I'm all in for adding cool features, if they bring more harm than good, I would rather keep them unavailable. Having a custom background for tldr is less worth than having a proper display and copy/paste. After all, users can set their own terminal background to anything they wish.

@MasterOdin
Copy link
Contributor

I think it's more that the node client used to set the background, and the background stuff was added to the python client to make it feature parity with the node client. Can always just remove it here and see if anyone complains and re-address this debate then.

@zlatanvasovic
Copy link
Contributor Author

@MasterOdin Can you come to Gitter for a moment?

@zlatanvasovic zlatanvasovic merged commit 3d710a9 into master Apr 30, 2020
@zlatanvasovic zlatanvasovic deleted the newline branch April 30, 2020 22:15
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.

Reduce empty lines in output

3 participants