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

Feature/format UI components #3193

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

Zykino
Copy link
Contributor

@Zykino Zykino commented Mar 10, 2024

Add ways to serialize/format ui components instead of only proposing print options.

I can remove the first commit if you want, but it did not work on my PC without it:

  1. rust -vV error when using the "previous" rust toolchain.
  2. protoc complain that optional is an experimental feature of the syntax3.

I also changed the interface to borrow the Text elements instead of owning it, so it can be reused.

I had some reflection on the ribbon: plugin developers will often want to have multiples of them in a row, so accepting an into_iter sounded nicer.
I also wanted to have a possibility to color the full line with the background color, but I’m not sure how to get the bg color from the user’s theme.

Note: I changed the session plugin as needed and others, but I did not run everything plugin before/after. However I did not see obvious breakage in the "after" state.

@imsnif imsnif self-assigned this Mar 11, 2024
@Zykino
Copy link
Contributor Author

Zykino commented Mar 11, 2024

Thinking about it again, I kind of want to have an SerializeAnsi trait that requires the equivalent of serialize_text_with_coordinates and serialize_text. Maybe another equivalent trait for iterable (useful for ribbon’s line and nested list’s view, maybe even tables?).

But in the same time I’m not sure it will help format/print elements. It may be too much into the "gui framework" side, which is apparently already hard to do (at least in rust).

What do you think ?

PS: I’m thinking of having SerializeAnsi be some trait that do the same as the Display trait. But in some sense having some setCursorPosition struct that once used with the trait don’t Display anything but move the cursor at the correct position. This let peoples creating an iterator of dyn SerializeAnsi that alternately display thing then move the cursor.

@imsnif
Copy link
Member

imsnif commented Mar 25, 2024

Hey @Zykino - sorry for taking so long to get to this and thanks for your patience!

Any chance we can weed this down to just adding a simple serialization interface? I appreciate the thought you put into code reuse as well as improving existing build issues and such, but I have to minimize the surface of changes due to the size of this project and the burden of maintainership. Thanks for understanding.

@Zykino Zykino force-pushed the feature/format-ui-components branch 2 times, most recently from 8ff6f72 to 499f081 Compare March 27, 2024 00:24
@Zykino
Copy link
Contributor Author

Zykino commented Mar 27, 2024

  • I removed the compilation commit (kept it in my fork so I can re-use it if needed).
  • Transformed the print_* function to take &references so we don’t need to clone elements if we want to use print them multiples times
  • Add serialize_* equivalent function that returns Strings
  • For the ribbon, I think it is interesting to give multiples items and print them in a line (like for the table, and the nested list), so I added serialize_ribbon_line*
  • Fixed the compilation where it now needs reference

@imsnif
Copy link
Member

imsnif commented Mar 28, 2024

hi @Zykino - thanks for your work and efforts on this. Can we please change this to just add the serialization interface without changing anything else?

@Zykino Zykino force-pushed the feature/format-ui-components branch 2 times, most recently from fe37fda to ef0d9eb Compare March 29, 2024 23:08
@Zykino Zykino force-pushed the feature/format-ui-components branch 2 times, most recently from 0b84261 to ad3b71c Compare April 2, 2024 21:23
@imsnif
Copy link
Member

imsnif commented Apr 5, 2024

Thanks for this and for the changes!

@imsnif imsnif merged commit b10ccb8 into zellij-org:main Apr 5, 2024
6 checks passed
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.

2 participants