lbonn / rofi

Rofi: A window switcher, run dialog and dmenu replacement - fork with wayland support
Other
963 stars 39 forks source link

[REQUEST] ABI compatibility with Rofi? #96

Closed SabrinaJewson closed 10 months ago

SabrinaJewson commented 11 months ago

Before opening a feature request

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: https://github.com/SabrinaJewson/rofi-mode.rs/issues/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 commented 11 months ago

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

@MoetaYuko FYI

moetayuko commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 https://github.com/lbonn/rofi/blob/5cd1e3ca2b6b896b2e591b465cc590479ef07aab/source/rofi-icon-fetcher.c#L326-L328 which requires separate original size and scale. Otherwise, we can simply multiply the size with scale before passing it.

mcepl commented 8 months ago

With respect to https://github.com/svenstaro/rofi-calc/issues/112 when we can expect new release, so that all those rebuilds happen?

lbonn commented 8 months ago

@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 commented 8 months ago

On Wed Jan 10, 2024 at 4:20 PM CET, lbonn wrote:

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.

Yeah, but the problem is that for example we in openSUSE have BOTH versions of rofi, so the questions is against which version of it, we should rebuild rofi-calc.

ret2src commented 5 months ago

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

jones-josh commented 2 months ago

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