achannarasappa / ticker

Terminal stock ticker with live updates and position tracking
GNU General Public License v3.0
5.02k stars 273 forks source link

Tags not showing on last entry. #158

Closed metalsp0rk closed 2 years ago

metalsp0rk commented 3 years ago

Describe the bug Tags are not showing for the last entry in ticker, when scrolling is applicable.

To Reproduce Steps to reproduce the behavior:

  1. launch with --show-tags
  2. Scroll to the bottom
  3. Observe that the last entry does not show the tags.

Expected behavior The interface will allow you to scroll down to the very bottom.

Screenshots image

Environment (please complete the following information):

Additional context If you resize the terminal window to increase the vertical size, the tags will show up, but if you scroll up one line, then try to scroll back down, it will not allow you to.

I suspect the key-scroll method has a bounds-check error. I'll dig into it when I have a chance and if I can figure it out, I'll submit a PR. Just thought I'd submit an issue to share my findings.

sergeevabc commented 3 years ago

@metalsp0rk, you’re lucky, in my case ticker.exe --show-tags displays a black screen with clock ticking, that’s all.

metalsp0rk commented 3 years ago

oh wow, I'm not sure what's going on there. You're on windows, I assume. I don't have access to a windows system to verify your error, so I'm not sure how much I'll be able to do about this.

However, are you using cmd or powershell to get this result? I'll see if I can get a coworker to confirm this.

achannarasappa commented 3 years ago

Thanks for reporting this @sergeevabc ! Looks this bug has re-emerged in a recent release.

Would very much appreciate a PR if you end up having time to take a look but if not I'll take a look at fixing it myself at some point.

sergeevabc commented 3 years ago

@achannarasappa, my profile says “bugs hunter”, while PR requires developer skills and tools. I’ll wait and verify when it’s ready.

achannarasappa commented 3 years ago

Meant to tag the author in this case @metalsp0rk :)

In any case thank you both for your input and details on the issue

metalsp0rk commented 3 years ago

I haven't yet had time to look into it, if anyone else wants to take lead on this, I'm fine with that. Otherwise, I'll take a look when I get some time.

github-actions[bot] commented 3 years ago

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

MarcoLucidi01 commented 3 years ago

hello, I'm a big fan of ticker, I use it daily to track my positions. I saw this "good first issue" and decided to help :)


what I found out is basically that when the content doesn't fit on a single page, github.com/charmbracelet/bubbles/viewport doesn't render the last line of content if it doesn't end with a new line \n character (bubbles/viewport is used in ticker's ui.Model).

I was able to reproduce the issue even with bubbletea's pager example that uses bubbles/viewport. here what I did:

$ git clone https://github.com/charmbracelet/bubbletea.git
...
$ cd bubbletea/examples/pager/
$ go build
$ ls -1
artichoke.md    # this file is displayed in the example
main.go
pager*
$ tail -1 artichoke.md | xxd | tail -1
00000030: 2069 6e20 5370 616e 6973 682e 0a          in Spanish..    # ends with new line
$ ./pager

https://user-images.githubusercontent.com/23704923/133423929-6b4b7251-8abe-44b5-b040-b663e53f63bf.mp4

everything looks fine, now I remove last new line \n character from artichoke.md:

$ truncate -s -1 artichoke.md
$ tail -1 artichoke.md | xxd | tail -1
00000030: 2069 6e20 5370 616e 6973 682e             in Spanish.
$ ./pager

https://user-images.githubusercontent.com/23704923/133424155-3faf68e6-2604-4019-b3ba-8b194e5a7435.mp4

and the last line

_Alcachofa_, if you were wondering, is artichoke in Spanish.

is not displayed anymore. the same thing is happening in ticker where the last line is the "tags line". as you can imagine, the issue is not directly related to --show-tags flag: it can happen with any "last line" not ending with a new line \n.

a quick-and-dirty fix for ticker would be to add a terminating new line \n to watchlist.View() (viewport's content), e.g.:

$ git diff
diff --git a/internal/ui/component/watchlist/watchlist.go b/internal/ui/component/watchlist/watchlist.go
index 00bdebe..65d56da 100644
--- a/internal/ui/component/watchlist/watchlist.go
+++ b/internal/ui/component/watchlist/watchlist.go
@@ -105,7 +105,7 @@ func (m Model) View() string {

        }

-       return grid.Render(grid.Grid{Rows: rows, GutterHorizontal: widthGutter})
+       return grid.Render(grid.Grid{Rows: rows, GutterHorizontal: widthGutter}) + "\n"
 }

 func getCellWidths(assets []c.Asset) cellWidthsContainer {

this works, but it's ugly. I'm pretty positive that the bug is in bubbles/viewport rather than in ticker. I think that not rendering the last line of content when it doesn't end with a new line \n is not expected behavior.

I tried to dig into bubbles/viewport/viewport.go, there are lot of places where they do len(m.lines)-1 i.e. subtract 1 from total number of lines. I didn't quite understand why it's necessary, but I suspect it's related to the bug.

anyway, I was thinking about opening an issue in bubbles/viewport reporting all this, what do you guys think?

achannarasappa commented 3 years ago

Thanks for doing that analysis and finding the likely source of the issue @MarcoLucidi01!

It sounds like the ideal place to make the fix would be bubbles/viewport and I'd suggest starting there. I'm eager to get this bug resolved as it's sitting unresolved for a while now and glad you were able to provide a lot more clarity on the problem.

MarcoLucidi01 commented 3 years ago

quick update: the bug has been fixed in bubbles/viewport's master and will be available in the next bubbles release:

https://github.com/charmbracelet/bubbles/pull/74 https://github.com/charmbracelet/bubbles/issues/73#issuecomment-922062551

github-actions[bot] commented 3 years ago

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

github-actions[bot] commented 2 years ago

This issue was closed because it has been stalled for 14 days with no activity.

meowgorithm commented 2 years ago

This fix is now available in Bubbles v0.10.0.