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

scroll text if width of Textbox is exceeded #4293

Merged
merged 7 commits into from
Oct 20, 2024

Conversation

bjarthur
Copy link
Contributor

@bjarthur bjarthur commented Aug 30, 2024

Description

Fixes #4268

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • Added an entry in CHANGELOG.md (for new features and breaking changes)
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@bjarthur bjarthur force-pushed the bja/scrolltextbox branch 2 times, most recently from f00a8b5 to ce7b505 Compare August 30, 2024 18:04
@bjarthur
Copy link
Contributor Author

not sure how to add a test. happy to do so if someone suggests a way

@bjarthur
Copy link
Contributor Author

again, could i please get a review? here's demo code and a movie of the amazing new functionality this PR offers to Makie :)

julia> using GLMakie

julia> f = Figure()

julia> Textbox(f[1,1], width=100)
Screen.Recording.2024-09-16.at.9.49.53.AM.mov

Copy link
Member

@jkrumbiegel jkrumbiegel left a comment

Choose a reason for hiding this comment

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

This should have a visual reference test I think. Textbox doesn't have its own one so a new one can be added in figures_and_makielayout.jl. All the branches that currently codecov complains about should be handled there, like one normal textbox, one with text scrolled.

With regards to the implementation, I guess this all could be handled a bit more cleanly by a translate! on the text and the cursor plot objects instead of modifying their input args. That would correspond more to the idea of "scrolling", too. I don't really like textplot.converted[1][] = [Point3f(oldv... as at some point it gets hard to reason about what influences what. Textbox is already not so easy to understand.

@bjarthur bjarthur force-pushed the bja/scrolltextbox branch 2 times, most recently from b952148 to 39bea74 Compare October 1, 2024 16:40
@bjarthur
Copy link
Contributor Author

bjarthur commented Oct 1, 2024

thanks for the suggestion regarding the implementation! i have refactored to use translate and indeed it is much more understandable. working on tests...

@bjarthur
Copy link
Contributor Author

bjarthur commented Oct 2, 2024

i have added visual reference tests, but am having trouble uploading them. when i click on "Update reference images with selection", a popup appears with a "v0.21.0" tag. irrespective of whether i stick with that or choose another (e.g. "v0.22.0", or "master") i get an error on the julia REPL. is this because i don't have write permissions to the required repo?

@jkrumbiegel
Copy link
Member

Yes you need commit rights to use the reference updater. Contributors can only add new test code which notifies committers in the PR status that new images are needed

@bjarthur
Copy link
Contributor Author

bjarthur commented Oct 3, 2024

thanks @jkrumbiegel for the info. let me finish the tests and i'll ping you for another review.

@bjarthur bjarthur force-pushed the bja/scrolltextbox branch 2 times, most recently from 7193ee2 to 896f515 Compare October 3, 2024 13:13
@bjarthur
Copy link
Contributor Author

bjarthur commented Oct 3, 2024

ready for review again.

codecov must not consider the reference tests as those three lines are definitely used there

@bjarthur
Copy link
Contributor Author

the ReferenceTests report that "Violin plots differently scaled.png" is different. how could that possibly be?

@SimonDanisch
Copy link
Member

That's not a problem of the pr ;) we've been updating the reference images and there's likely a Missmatch

@jkrumbiegel jkrumbiegel merged commit 67a4c69 into MakieOrg:master Oct 20, 2024
18 checks passed
@SimonDanisch
Copy link
Member

Thanks :)

@bjarthur bjarthur deleted the bja/scrolltextbox branch October 21, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

scroll text if width of Textbox is exceeded
4 participants