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

Fixed a bug with flickering if scrollbar size is set to 0. #4460

Closed
wants to merge 2 commits into from
Closed

Fixed a bug with flickering if scrollbar size is set to 0. #4460

wants to merge 2 commits into from

Conversation

lukasmarer
Copy link

Basically in the textual website - https://textual.textualize.io/styles/scrollbar_size/ there is:

Tip
If you want to hide the scrollbar but still allow the container to scroll using the mousewheel or keyboard, you can set the scrollbar size to 0.

But when I tried it it would just flicker or not show up at all. Now if you want to hide a scrollbar you can use any of these:

scrollbar-size: 0 0;  # hide both scrollbars
scrollbar-size-horizontal: 0;  # hide horizontal scrollbar
scrollbar-size-vertical: 0;  # hide vertical scrollbar

It was a small change. Just a single line of code. I don't understand what exactly the change I made did but with trial and error I managed to fix it without knowing the internals of textual. I am pretty sure I didn't break anything since it is just a small change. I tested it with a small application and it worked so it should be good to go.
(If I did something wrong please tell me this is my first time contributing to a project. I am open to learn)

Now setting scrollbar-size to 0 will work and won't flicker
fix typo: vertival updated to vertical
@willmcgugan
Copy link
Collaborator

That may very well be the right fix!

@lukasmarer
Copy link
Author

why does it say Some checks were not successful? I don't think that's good. I tried looking at the output or something but I don't understand it. Did I break something?

@willmcgugan
Copy link
Collaborator

You should be able to click "Details" to see what failed.

The formatting check failed. That's easily fixed by installing and running black.

And a test failed. This could mean your change broken something else.

You can run the tests locally with pytest.

@lukasmarer
Copy link
Author

Thanks a lot! And when I format it and fix the errors it is causing in testing - do I create a new pull request and close this one?

@willmcgugan
Copy link
Collaborator

No, just make changes, and push. That will make the tests run again...

@willmcgugan
Copy link
Collaborator

Do you have an MRE (short example) that shows the issue? I can't reproduce it with the following code:

from textual.app import App, ComposeResult
from textual.widgets import Static

class SApp(App):
    CSS = """
    Screen {
        scrollbar-size-vertical: 0;
    }
    """
    def compose(self) -> ComposeResult:
        yield Static("\n".join(str(n)*20 for n in range(100)))


if __name__ == "__main__":
    app = SApp()
    app.run()

@lukasmarer
Copy link
Author

lukasmarer commented May 1, 2024

The following code reproduces the issue for me.

from textual.app import App
from textual.containers import ScrollableContainer
from textual.widgets import Static


class MyApp(App):
    CSS = """
    Screen {
        layout: horizontal;
    }
    
    ScrollableContainer {
        scrollbar-size-vertical: 0;
    }
    
    #container {
        width: 10%;
        background: #202020;
    }
    """
    
    def compose(self):
        with ScrollableContainer(id="container"):
            for i in range(50):
                yield Static("rast st srar cvd"*15)
        with ScrollableContainer():
            yield Static("arsta")

if __name__ == "__main__":
    app = MyApp()
    app.run()

However I gave up trying to fix the "issues" that occur during the automatic tests. That is because I git cloned the official repository, run all of the setup commands and downloaded everything necessary as stated in the CONTRIBUTING.md file. Then I ran the tests using make test (again as stated in CONTRIBUTING.md) and after about 2 and a half minutes it said that there were some errors. I do not remember how many errors there were but it was more than 0. As I do not currently have the will and don't fell like fixing all of those issues and potentially issues that I made with my contribution I gave up.

I might have done something wrong during setup but I have double checked and followed CONTRIBUTING.md closely. It is more then likely I think that I did something wrong during setup or during running the tests because it can't be that a repository that automatically checks every pull request using automated tests has content that fails those tests. If you know why I got the errors when I ran the tests on the unchanged official repository than please let me know.

If you feel like you want to contribute to this project and maybe fix the issues you are welcome. If not, I will close this pull request and report the bug and the protentional fix that I found.

Also thanks for all of the help that you gave me. I am new and it really helped me out a lot. It's just that I got errors even from the unchanged repository when running tests that deterred me. Even then it's still a great and a new experience contributing to open source.

@TomJGooding
Copy link
Contributor

I ran the tests using make test [...] and it said that there were some errors.

By "errors" do you mean that some of tests failed?

The following code reproduces the issue for me.

I can't reproduce the issue you're describing with this code. What version of textual are you using?

@lukasmarer
Copy link
Author

lukasmarer commented May 1, 2024

I am using python 3.11.1 and Textual 0.40.0. I now realized I was in fact running an outdated version of textual. I updated to 0.58.1. I thought I had the latest version. When I updated the issue disappeared.

By "errors" do you mean that some of tests failed?

Yes. I forgot the word FAILED because I ran the test a long time ago (for me).

I ran the tests again and got: (I removed all of those extra equals signs.)

= short test summary info =
FAILED tests/test_suspend.py::test_suspend_supported - OSError: [WinError 6] The handle is invalid
= 1 failed, 2802 passed, 4 xfailed, 3 warnings in 290.72s (0:04:50) =

Only one test failed, which is weird. I swear it was at least 3 test before.

Also I am sorry for not checking my Textual version before making a pull request. At least it won't happen again... probably.

@TomJGooding
Copy link
Contributor

This is why it is best practice to create an issue first with an MRE (short example) before making a pull request. Especially as the issue template would have prompted you for the version you're running.

I don't want to discourage you from contributing to open source, but you should at least run the tests locally to ensure your changes haven't broken anything before submitting a PR.

Thanks for providing the failure info, this might be something where Windows hasn't been taken into account.

@lukasmarer lukasmarer closed this May 2, 2024
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.

3 participants