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

feat: support customizable alignment for image display #1897

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gaesa
Copy link
Contributor

@gaesa gaesa commented Nov 10, 2024

This is an attempt to provide a possible solution to #1141.

  • Adds configuration options in yazi.toml for setting preferred image alignment, which plugin developers can optionally respect.
  • Updates plugin defaults to use PREVIEW.alignment for aligned image display.
  • Ensures backward compatibility for existing plugins.

It appears to work well overall, though some aspects remain unclear:

  1. Does the current use of serde align with usual practices?
  2. Are the new Lua API function consistent with current design philosophy?

@sxyazi
Copy link
Owner

sxyazi commented Nov 10, 2024

Thanks for the PR!

Could you please just change the key parts and pull out the refactoring into a new PR? That would make the review a bit easier.

gaesa added a commit to gaesa/yazi that referenced this pull request Nov 11, 2024
Exposing `image_area` separately allows future Lua integration to handle
image placement more flexibly, without needing to invoke full rendering.
This change promotes modularity and simplifies obtaining image dimensions alone.

This refactor provides the foundation for feature PR sxyazi#1897, preparing for easier integration.
@gaesa gaesa force-pushed the alignment branch 2 times, most recently from 2fad7d8 to 37a19b6 Compare November 12, 2024 02:13
@gaesa
Copy link
Contributor Author

gaesa commented Nov 12, 2024

This PR currently introduces the ability to customize the alignment for image display by adding a third parameter to the image_show function. Here’s a breakdown of key considerations:

  1. Parameter Type Selection: Using Offset Instead of Alignment
    Offset was chosen for the new parameter because it provides more granular control over image placement than an alignment setting would allow. This gives greater flexibility to the Lua API.

  2. Using Table for the Lua API image_show's 3rd Parameter
    Instead of creating a specific Lua type for Offset, a Table was used instead. Implementing a custom type would add complexity, and with limited familiarity with mlua, the added benefit of this approach was unclear.

  3. Not Choosing ratatui::layout::Offset
    ratatui::layout::Offset relies on i32, which would add unnecessary complexity here, as x and y are initially defined from the top-left corner of the preview area.

  4. Location of Offset Definition
    Placing the Offset definition in yazi-adapter instead of yazi-shared makes it easier to interact with the global PREVIEW variable.

  5. Using Option<Offset> as the image_show's 3rd Parameter Type
    Alignment requires the sizes of both the image and max, but this information is unavailable at the time Default::default is called. Therefore, Option<Offset> was used, as implementing Default for Offset is impractical in this case.

  6. Calling Offset::from in Multiple Places Instead of Exclusively in image_show of adapter.rs
    Similar to the 5th point, while max’s size is available here, the image size remains unknown at this stage, making it impractical to use Offset::from. Even if we were to extract image_area from image_show, avoid exposing image_area, and pass the data it returns to image_show — using image_area’s result to compute the offset in the meantime — it would make the signatures of different image_show functions inconsistent and introduce extra complexity, which could negate the benefits of calling Offset::from in adapter.rs.

  7. Lua’s Access to Image Size
    While allowing Lua access to image size would enable custom image placement logic (avoiding workarounds like in mediainfo.yazi and improving mixed previews for similar plugins), it could lead to redundant calculations when applying the resulting size to display the image. Since the purpose of obtaining the image size is for custom placement, one possible solution is for Lua to pass a function to Rust: Lua would provide mapping logic for the image and preview area sizes, which Rust would apply, resulting in an offset for display. However, it is unclear whether this approach is feasible, especially in an async environment, and the potential cost is also uncertain.


Next Steps

Open to feedback on improving specific implementation details or considering alternative approaches.

@sxyazi sxyazi force-pushed the main branch 10 times, most recently from f2e33da to c65bdb3 Compare November 14, 2024 05:08
@sxyazi sxyazi force-pushed the main branch 10 times, most recently from 2b70f8d to 5e48df5 Compare November 26, 2024 09:11
@sxyazi sxyazi force-pushed the main branch 2 times, most recently from 43473db to d72f903 Compare November 30, 2024 13:26
@gaesa gaesa force-pushed the alignment branch 2 times, most recently from 5324b56 to 88998fa Compare December 7, 2024 15:28
@sxyazi sxyazi force-pushed the main branch 3 times, most recently from 1394fb4 to 62ac224 Compare December 9, 2024 02:28
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