egui_winit: Make the clipboard_text and allow_ime state public

[tosti007]

The egui_winit::State contains both the clipboard and allow_ime, but these are both private fields. In our implementation we use egui_winit to do most of the lifting, but it would come really in handy if we could access these values.

Instead of making the fields pub we opted for separate get/set functions, that way calling side can't freely replace any of the values fully and it's more in line with the style of the rest of the codebase.

[tedsteen]

Instead of making the fields pub I opted for getter functions, such that the calling side is discouraged to freely alter the State's internal data.

but the return type is &mut allowing for mutation of the internal data?

[MarijnS95]

allow_ime needs to be mut to be able to tell State that we've already called set_ime_enabled(). This wasn't/isn't conveyed in the original message but is what we need these changes for.

[tedsteen]

allow_ime needs to be mut to be able to tell State that we've already called set_ime_enabled(). This wasn't/isn't conveyed in the original message but is what we need these changes for.

It was a comment on the fact that the fields where private instead of public. I don't know what the convention is in this repo but I would personally make the fields public instead of writing methods that return a &mut

[MarijnS95]

@tedsteen that comment was written with immutability in mind, but even after correcting that we need mutable access to this field, this matches the fn egui_input()/fn egui_input_mut() pattern. Those functions are already available on State.

[emilk]

The difference is that egui_input_mut returns a mutable reference to a big struct. For a simple bool, a set_ function feels a lot more idiomatic imho.

[MarijnS95]

The difference is that egui_input_mut returns a mutable reference to a big struct. For a simple bool, a set_ function feels a lot more idiomatic imho.

Wasn't suggesting that a _mut() API is better than _set() for bools (though it allows us to get a mutable reference exactly once), just that the OP comment no longer holds (because we exactly need to freely alter the States internal data). We might also make both (allow_ime and egui_input) fields pub and do away with the API though.

[MarijnS95]

Ouch, looks like this missed out on the 0.25 release

[tosti007]

The difference is that egui_input_mut returns a mutable reference to a big struct. For a simple bool, a set_ function feels a lot more idiomatic imho.

I opted to take the same approach regarding the clipboard in the last commit, by making a clipboard and set_clipboard function. This way the backing implementation is also free to change if needed in the future.

Ouch, looks like this missed out on the 0.25 release

Sorry, this had slipped my mind after the Christmas holiday D:

[MarijnS95]

Should we document why this is mutable?

[tosti007]

TBH it's something deep down inside arboard (not sure about smithay), and it isn't obviously documented there other than it being "and get operation" and doing any operation probably requiring mut access.

[MarijnS95]

@emilk @tedsteen any further feedback here?

[tedsteen]

@emilk @tedsteen any further feedback here?

No