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

[REQUEST] ABI compatibility with Rofi? #96

Closed
2 tasks done
SabrinaJewson opened this issue Nov 2, 2023 · 9 comments
Closed
2 tasks done

[REQUEST] ABI compatibility with Rofi? #96

SabrinaJewson opened this issue Nov 2, 2023 · 9 comments

Comments

@SabrinaJewson
Copy link

Before opening a feature request

  • I checked the next branch to see if the feature has already been implemented
  • I searched existing reports to see if it is already requested.

What is the user problem or growth opportunity you want to see solved?

Plugins built for normal Rofi are currently ABI-incompatible with plugins built for this (Wayland) Rofi; however, this is not made obvious to the user (attempting to load a Rofi plugin using Wayland Rofi simply results in UB, manifesting as an unhelpful assertion failure), nor is it easy for plugin authors to support both normal and Wayland Rofi.

I would suggest one of two things:

  1. undo the breaking changes made to Rofi’s plugin ABI (most importantly, the extra parameter in _mode_get_icon, but rofi_icon_fetcher_query[_advanced] may also be changed), or if that is not possible
  2. bump the ABI version number from 7 to something like 1007, to ensure that the error is made obvious to the user.

How do you know that this problem exists today? Why is this important?

Some people use Wayland Rofi for practical use, as it is in the AUR.

My interest in this comes from wanting to support Wayland Rofi in my Rust library for writing Rofi plugins: SabrinaJewson/rofi-mode.rs#8.

Who will benefit from it?

People who write and use plugins for Wayland Rofi.

Rofi version (rofi -v)

Latest commit from repositories, as of 2023-10-31

Configuration

N/A

Additional information

No response

@lbonn
Copy link
Owner

lbonn commented Nov 2, 2023

Aw good point, thank you. I think this is mainly a problem after #82

@moetayuko FYI

@moetayuko
Copy link

I didn't think about ABI compatibility at all. Which APIs need to align with the original rofi, is there sth like a list?

@SabrinaJewson
Copy link
Author

Everything that affects /usr/include/rofi needs to be API-comptible: helper.h, mode-private.h, mode.h, rofi-icon-fetcher.h, rofi-types.h. Randomly, rofi_view_reload from view.h also needs to be API-compatible.

The particular changes that are relevant here are in mode-private.h:

 typedef cairo_surface_t *(*_mode_get_icon)(const Mode *sw,
                                            unsigned int selected_line,
-                                           unsigned int height);
+                                           unsigned int height,
+                                           guint scale);

the corresponding function in mode.h:

 cairo_surface_t *mode_get_icon(Mode *mode, unsigned int selected_line,
-                               unsigned int height);
+                               unsigned int height, guint scale);

and the API of the Rofi icon fetcher:

-uint32_t rofi_icon_fetcher_query(const char *name, const int size);
+uint32_t rofi_icon_fetcher_query(const char *name, const int size,
+                                 const guint scale);

and

-uint32_t rofi_icon_fetcher_query_advanced(const char *name, const int wsize,
-                                          const int hsize);
+                                          const int hsize, const guint scale);

Technically, the addition of the scale field to the RofiImage struct in rofi-types.h is also a breaking change, but I don’t think that struct is actually in used in any other public APIs so it matters less.

I’m not really sure what the scale parameter does / how necessary it is, so I don’t know what the best decision to make here is.

@moetayuko
Copy link

I’m not really sure what the scale parameter does / how necessary it is, so I don’t know what the best decision to make here is.

The main usecase is

icon_path = icon_path_ = nk_xdg_theme_get_icon(
rofi_icon_fetcher_data->xdg_context, themes, NULL, sentry->entry->name,
MIN(sentry->wsize, sentry->hsize), sentry->scale, TRUE);
which requires separate original size and scale. Otherwise, we can simply multiply the size with scale before passing it.

moetayuko added a commit to moetayuko/rofi that referenced this issue Nov 5, 2023
lbonn pushed a commit that referenced this issue Nov 30, 2023
This time, preserving ABI compatibility with upstream rofi

Fixes #96
@lbonn lbonn closed this as completed in 36621af Nov 30, 2023
@mcepl
Copy link

mcepl commented Jan 10, 2024

With respect to svenstaro/rofi-calc#112 when we can expect new release, so that all those rebuilds happen?

@lbonn
Copy link
Owner

lbonn commented Jan 10, 2024

@mcepl distros should update rofi-calc when a new version of upstream rofi is released.

Right now the wayland branch is using the ABI from rofi:next but it is different than the one in the last rofi release, 1.7.5, so releasing a new rofi-wayland version would not help.

Until rofi's next version is released (1.7.6?), I don't think there is an easy fix except for recompiling rofi-calc.

@mcepl
Copy link

mcepl commented Jan 10, 2024 via email

@ret2src
Copy link

ret2src commented May 7, 2024

Quick and dirty fix for Arch users: Install aur/rofi-calc-git

@jones-josh
Copy link

Likewise confirming for rofi-emoji that its git version (i.e. the AUR mirror for arch users) at commit aaa8ab works against 1.7.5+wayland3 also.

manifesting as an unhelpful assertion failure

Unsure how this varies system to system, but I saw the below. This to me was obvious of some breaking change between versions and lead me here. Perhaps including the below string will aid others to whom it's less obvious.

Rofi-WARNING **: 13:53:00.343: ABI version of plugin: 'emoji.so' does not match: 00000006 expecting: 00000007

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 a pull request may close this issue.

6 participants