gtk: Implementable implementation for AccessibleText #1789

felinira · 2024-07-04T13:23:38Z

No description provided.

felinira · 2024-07-04T13:30:53Z

gtk4/src/subclass/accessible_text.rs

+        let mut c_ranges = glib::Slice::with_capacity(attrs.len());
+        let mut c_names = glib::StrV::with_capacity(attrs.len());
+        let mut c_values = glib::StrV::with_capacity(attrs.len());
+
+        for (range, name, value) in attrs {
+            c_ranges.push(range);
+            c_names.push(name);
+            c_values.push(value);
+        }
+
+        *ranges = c_ranges.to_glib_container().0;
+        *attribute_names = c_names.into_glib_ptr();
+        *attribute_values = c_values.into_glib_ptr();


I don't like having to juggle the collections like that here, and in get_default_attributes, but having to return three Vec from this function would also be odd. Semantically these are triples, but they are supposed to be stored in three different arrays.

A wrapper type could solve the problem but I'm not sure we should introduce two of these just for this one-off.

felinira · 2024-07-07T19:20:40Z

I am a little unsure if it's ever ok to return null in get_contents(_at) or not. Should I return an empty gbytes instead?

bilelmoussaoui · 2024-07-10T09:29:55Z

I don't have the time to review this right now but two questions:

is it important for you to have this in the next release or waiting till a future .x would be okay?
do you think you could add an example of how it could be used?

felinira · 2024-07-10T10:16:16Z

is it important for you to have this in the next release or waiting till a future .x would be okay?

We can postpone it for now.

do you think you could add an example of how it could be used?

Sure, that makes sense. It's quite an involved interface.

gtk4/src/accessible_text_range.rs

gtk4/src/subclass/accessible_text.rs

gtk4/src/accessible_text_range.rs

gtk4/src/subclass/accessible_text.rs

sdroege · 2024-10-25T14:09:33Z

gtk4/src/subclass/accessible_text.rs

+                &mut attribute_names,
+                &mut attribute_values,


I don't think there are necessarily n_ranges attribute names/values. Both are just NULL-terminated string arrays. I'd assume both have the same length but otherwise they should be independent from the ranges

I think there are. See https://docs.gtk.org/gtk4/vfunc.AccessibleText.get_attributes.html

Each attribute is composed by:

a range

a name

a value

You have a buffer of text, and then you annotate a range with attribute / value pairs. If you had more names and values, where would the extra attributes apply to?

See https://gitlab.gnome.org/GNOME/gtk/-/blob/main/gtk/a11y/gtkatspitext.c#L173 for an implementation. It even calls n_ranges n_attrs 🙈 .

I generally think this interface is quite confusing in that regard though, and it's very easy to get lost here. Which is the main reason why I've opted for the approach of only allowing triples here. Because having less attribute/name pairs than ranges actually immediately triggers a memory unsafety bug just in the code linked above.

sdroege · 2024-10-25T14:11:05Z

gtk4/src/subclass/accessible_text.rs

+        }
+    }
+
+    fn parent_contents(&self, start: u32, end: u32) -> Option<glib::Bytes> {


I think this must not return NULL? Or at least the C functions is not annotated as such

I used the upstream implementation as reference. Yes, I think this might be an annotation issue. https://gitlab.gnome.org/GNOME/gtk/-/blob/main/gtk/gtkaccessibletext.c#L20

Would it be too much to ask if you could fix those missing nullable annotations? otherwise i can take care of that :)

sdroege · 2024-10-25T14:11:43Z

gtk4/src/subclass/accessible_text.rs

+                end.as_mut_ptr(),
+            );
+
+            if !bytes.is_null() {


Not marked as nullable either

Yeah, another annotation issue. If I don't do this I get UB. https://gitlab.gnome.org/GNOME/gtk/-/blob/main/gtk/gtkaccessibletext.c#L35

gtk4/src/subclass/accessible_text.rs

sdroege · 2024-10-25T14:14:09Z

gtk4/src/subclass/accessible_text.rs

+
+        bytes.to_glib_full()
+    } else {
+        std::ptr::null_mut()


Not marked as nullable in C

https://github.com/gtk-rs/gtk4-rs/pull/1789/files#r1817872489

gtk4/src/subclass/accessible_text.rs

felinira · 2024-10-26T16:42:02Z

There is still one thing here that I don't particularly understand: The 'bytes' return value is supposed to be a glib::Bytes that sometimes contains '\0' bytes and sometimes it doesn't. calling contents() on gtk::TextView will return a \0 byte, but it won't with contents_at().

examples/accessible_text/main.rs

examples/accessible_text/text_view.rs

felinira · 2024-10-26T17:02:10Z

Maybe two wrapper objects for (range, attr, value) triples wouldn't be such a bad idea. When writing the example it was very cumbersome to change attributes, adding attributes by constants, etc.

bilelmoussaoui · 2024-10-26T17:16:57Z

Maybe two wrapper objects for (range, attr, value) triples wouldn't be such a bad idea. When writing the example it was very cumbersome to change attributes, adding attributes by constants, etc.

I think we can do that as a second step :) but yes, having to deal with constants is not ideal. But also not sure how we can do something that wouldn't be too far away from the original API.

Maybe we can make those attributes an enum? then you would only have a Vec<(Range, TextAttribute)>

…for implementing AccessibleText

bilelmoussaoui · 2024-10-30T20:23:02Z

examples/accessible_text/text_view.rs

+    impl AccessibleTextView {}
+}
+
+glib::wrapper! {
+    pub struct AccessibleTextView(ObjectSubclass<imp::AccessibleTextView>)
+    @extends gtk::Widget, gtk::TextView,
+    @implements gtk::Accessible, gtk::AccessibleText, gtk::Buildable, gtk::ConstraintTarget, gtk::Scrollable;
+}
+
+impl AccessibleTextView {}


Suggested change

impl AccessibleTextView {}

}

glib::wrapper! {

pub struct AccessibleTextView(ObjectSubclass<imp::AccessibleTextView>)

@extends gtk::Widget, gtk::TextView,

@implements gtk::Accessible, gtk::AccessibleText, gtk::Buildable, gtk::ConstraintTarget, gtk::Scrollable;

}

impl AccessibleTextView {}

}

glib::wrapper! {

pub struct AccessibleTextView(ObjectSubclass<imp::AccessibleTextView>)

@extends gtk::Widget, gtk::TextView,

@implements gtk::Accessible, gtk::AccessibleText, gtk::Buildable, gtk::ConstraintTarget, gtk::Scrollable;

}

bilelmoussaoui · 2024-10-31T14:34:56Z

gtk4/src/subclass/accessible_text.rs

+        }
+    }
+
+    fn parent_contents(&self, start: u32, end: u32) -> Option<glib::Bytes> {


Would it be too much to ask if you could fix those missing nullable annotations? otherwise i can take care of that :)

felinira requested a review from bilelmoussaoui as a code owner July 4, 2024 13:23

felinira commented Jul 4, 2024

View reviewed changes

felinira force-pushed the wip/fina/accessible-text-impl branch from dadc102 to d3daa8a Compare July 7, 2024 18:08

bilelmoussaoui reviewed Jul 15, 2024

View reviewed changes

bilelmoussaoui requested review from bilelmoussaoui and sdroege October 25, 2024 06:57

bilelmoussaoui reviewed Oct 25, 2024

View reviewed changes

gtk4/src/subclass/accessible_text.rs Show resolved Hide resolved

gtk4/src/subclass/accessible_text.rs Outdated Show resolved Hide resolved

gtk4/src/subclass/accessible_text.rs Outdated Show resolved Hide resolved

gtk4/src/subclass/accessible_text.rs Outdated Show resolved Hide resolved

sdroege reviewed Oct 25, 2024

View reviewed changes

gtk4/src/accessible_text_range.rs Show resolved Hide resolved

sdroege reviewed Oct 25, 2024

View reviewed changes

gtk4/src/subclass/accessible_text.rs Outdated Show resolved Hide resolved

sdroege reviewed Oct 25, 2024

View reviewed changes

gtk4/src/subclass/accessible_text.rs Outdated Show resolved Hide resolved

sdroege reviewed Oct 25, 2024

View reviewed changes

gtk4/src/subclass/accessible_text.rs Outdated Show resolved Hide resolved

felinira force-pushed the wip/fina/accessible-text-impl branch from d3daa8a to bd836ee Compare October 26, 2024 16:37

felinira force-pushed the wip/fina/accessible-text-impl branch from bd836ee to 54881a8 Compare October 26, 2024 16:58

bilelmoussaoui reviewed Oct 26, 2024

View reviewed changes

examples/accessible_text/main.rs Outdated Show resolved Hide resolved

examples/accessible_text/text_view.rs Outdated Show resolved Hide resolved

felinira force-pushed the wip/fina/accessible-text-impl branch 2 times, most recently from 09c571c to 90c127c Compare October 26, 2024 17:07

felinira force-pushed the wip/fina/accessible-text-impl branch from 90c127c to b2798f7 Compare October 26, 2024 17:56

felinira added 3 commits October 30, 2024 15:01

gtk/accessible_text_range: Convert to boxed object to make it usable …

4a34051

…for implementing AccessibleText

gtk: Implementable implementation for AccessibleText

7e48281

examples: Add missing examples to README.md

19a71f9

felinira force-pushed the wip/fina/accessible-text-impl branch from b2798f7 to df3268e Compare October 30, 2024 14:06

bilelmoussaoui reviewed Oct 31, 2024

View reviewed changes

examples: Add example for custom AccessibleText implementations

d86d9dd

felinira force-pushed the wip/fina/accessible-text-impl branch from df3268e to d86d9dd Compare October 31, 2024 18:23

bilelmoussaoui merged commit 3909afb into gtk-rs:main Oct 31, 2024
40 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gtk: Implementable implementation for AccessibleText #1789

gtk: Implementable implementation for AccessibleText #1789

felinira commented Jul 4, 2024

felinira Jul 4, 2024

felinira commented Jul 7, 2024

bilelmoussaoui commented Jul 10, 2024

felinira commented Jul 10, 2024

sdroege Oct 25, 2024

felinira Oct 26, 2024

sdroege Oct 25, 2024

felinira Oct 26, 2024

bilelmoussaoui Oct 31, 2024

sdroege Oct 25, 2024

felinira Oct 26, 2024

sdroege Oct 25, 2024

felinira Oct 26, 2024

felinira commented Oct 26, 2024

felinira commented Oct 26, 2024

bilelmoussaoui commented Oct 26, 2024

bilelmoussaoui Oct 30, 2024

bilelmoussaoui Oct 31, 2024

gtk: Implementable implementation for AccessibleText #1789

gtk: Implementable implementation for AccessibleText #1789

Conversation

felinira commented Jul 4, 2024

Choose a reason for hiding this comment

felinira commented Jul 7, 2024

bilelmoussaoui commented Jul 10, 2024

felinira commented Jul 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felinira commented Oct 26, 2024

felinira commented Oct 26, 2024

bilelmoussaoui commented Oct 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment