Accessibility support on GTK

[Colomban Wendling]

Hi,

I'm trying to get (at least basic) accessibility support on the GTK
platform, and it's getting there: Orca can read the contents similar to
GtkTextView. However, I have a few questions now I better see where I'm
going:

* Would a platform-dependent (GTK only) accessibility layer be accepted,
at least until a platform-independent abstraction gets introduced for that?
I must say that I don't have enough knowledge or interest in the Windows
and MacOS platforms to try and get something generic. If it's a
requirement, I'll need to find a lot more enthusiasm.

By the way, I feel like maybe more than 30% of the code I wrote is
platform-specific, and the rest is gathering the information the
platform wants in the way it wants it. I didn't have to alter any
non-platform code, and the only changes yet to the ScintillaGTK
implementation are to plug the accessibility layer in.
So, this somewhat makes me wonder whether there would really be
something generic to write in the first place, because it'll get down to
what the accessibility APIs on the platform expect. Maybe they all want
roughly the same information in a similar way, but depending on how
different it is, it might not be very useful to have a generic layer if
each platform still ends up having to significantly alter the results.

* GTK uses ATK for accessibility, so the version I'm working on
implements the necessary ATK interfaces. So I'm wondering, would
implementation for the various ATK methods be something interesting at a
more generic level, or is it specific to GTK? I'm not sure how e.g. Qt
or wxWidgets does it, but if they do use ATK more or less directly, it
probably would be interesting to at least share some of the
implementation (basically, most of the actual information retrieval).

I'll post the patch in the coming days anyway, once I polished it a
little, and implemented the few missing bits; but at least the second
point might alter the implementation significantly.

Regards,
Colomban

[Neil Hodgson]

Colomban Wendling:

> * Would a platform-dependent (GTK only) accessibility layer be accepted,
> at least until a platform-independent abstraction gets introduced for that?

That would be OK. It would be better if the common cross-platform requirements were understood before design but that would require much effort from someone.

> I must say that I don't have enough knowledge or interest in the Windows
> and MacOS platforms to try and get something generic.

The API technology and the particular calls and attributes look much different between platforms. Windows uses COM for Microsoft UI Automation (UIA) which is aimed at test automation as well as accessibility.

https://en.wikipedia.org/wiki/Microsoft_UI_Automation

An example of one of the interfaces that should be implemented for UIA is ITextRangeProvider

https://msdn.microsoft.com/en-us/library/ee671377(v=vs.85).aspx

For Cocoa, NSAccessibility is the base protocol although some of the required features are already implemented reasonably closely for other purposes (Asian IME mostly) through NSTextInputClient.

https://developer.apple.com/reference/appkit/nsaccessibility

> So, this somewhat makes me wonder whether there would really be
> something generic to write in the first place, because it'll get down to
> what the accessibility APIs on the platform expect. Maybe they all want
> roughly the same information in a similar way, but depending on how
> different it is, it might not be very useful to have a generic layer if
> each platform still ends up having to significantly alter the results.

Its possible but other features have led to common code. For example, IMEs require ways to mark ranges and perform temporary changes and this has been made common.

> * GTK uses ATK for accessibility, so the version I'm working on
> implements the necessary ATK interfaces. So I'm wondering, would
> implementation for the various ATK methods be something interesting at a
> more generic level, or is it specific to GTK?

ATK seems to mostly be specific to GTK+. At one point Sun were implementing ATK for Java but that may have been related to using GTK+ with Java.

> I'm not sure how e.g. Qt
> or wxWidgets does it, but if they do use ATK more or less directly, it
> probably would be interesting to at least share some of the
> implementation (basically, most of the actual information retrieval).

Does ATK work on GTK+ on Win32?

Qt tries to abstract the base platforms:

http://doc.qt.io/qt-5/accessible.html

Its notable that it only supports the old deprecated MSAA interface on Win32 and on Unix it supports AT-SPI rather than ATK.

Neil

[Colomban Wendling]

Le 25/09/2016 à 03:00, Neil Hodgson a écrit :
>
>> * Would a platform-dependent (GTK only) accessibility layer be
>> accepted, at least until a platform-independent abstraction gets
>> introduced for that?
>
> That would be OK. It would be better if the common cross-platform
> requirements were understood before design but that would require
> much effort from someone.

Yes. Also, if people are interested in providing platform-specific
implementation, maybe it will be a little easier to see what is commonly
required. However it of course leads to (re)writing more code.

Note that if at some point there is an effort to get a unified layer,
I'd be happy to help on the GTK part.

>> I must say that I don't have enough knowledge or interest in the
>> Windows and MacOS platforms to try and get something generic.
>

> […]

>
> An example of one of the interfaces that should be implemented for
> UIA is ITextRangeProvider
>
> https://msdn.microsoft.com/en-us/library/ee671377(v=vs.85).aspx
>
> For Cocoa, NSAccessibility is the base protocol although some of the
> required features are already implemented reasonably closely for
> other purposes (Asian IME mostly) through NSTextInputClient.
>
> https://developer.apple.com/reference/appkit/nsaccessibility

Those interfaces look roughly similar to ATK's:
https://developer.gnome.org/atk/stable/AtkText.html

But they also seem to all have their own representation of the data, and
partially different expectation on it, which is the main thing I had to
do: retrieve the data in the appropriate format.

I guess another similar thing would be the events the accessibility part
wants to know about (text insertion/removal, attributes changes, caret
move, selection changes, etc.), but that too would have to be more
carefully evaluated.

>> I'm not sure how e.g. Qt or wxWidgets does it, but if they do use
>> ATK more or less directly, it probably would be interesting to at
>> least share some of the implementation (basically, most of the
>> actual information retrieval).
>
> Does ATK work on GTK+ on Win32?

That is a good question. I expect it would, but I didn't try yet.

> Qt tries to abstract the base platforms:
>
> http://doc.qt.io/qt-5/accessible.html
>
> Its notable that it only supports the old deprecated MSAA interface
> on Win32 and on Unix it supports AT-SPI rather than ATK.

AT-SPI is client-side layer of the GNOME accessibility (the layer ATs
use), and ATK is a library applications use to provide the data, so it's
not two totally different things. But anyway, if Qt has its own
abstraction layer for providing the data, it probably at least doesn't
expose ATK interfaces.

Colomban

[Colomban Wendling]

> I'll post the patch in the coming days anyway, once I polished it a
> little, and implemented the few missing bits; but at least the second
> point might alter the implementation significantly.

Here is an initial patch that give initial working support for at least
the most naive part of accessibility.

It obviously needs a lot of testing, especially from people familiar
with various ATs, which can better assess whether it works as they expect.

I don't expect this to be the final version, but it's usable and I'd
fancy testing, comments and alike.

Known issues:

* When the document associated with a view changes, Orca doesn't react.
It is especially problematic in SciTE which has one single view for all
documents. However, the same happens with GtkTextView, so I'm not sure
what to think.
I do emit contents change notifications, so supposedly ATs could pick it
up, but that would need to be investigated.

* It currently reports positions as Scintilla reports them (i.e. in
bytes), while ATK expects character count. I don't yet have assessed
the impact, but it's likely it should be fixed. It however might be
slightly problematic when having to locate the Nth character in the
buffer. Is there a cached way to query that, that don't mean reading
the whole buffer from the start?

* Similarly, it currently no encoding conversion is applied, although
ATK expects UTF-8. This will mean converting everywhere, and probably
will require fixing the point above about returning character counts
rather than bytes, because then every length and positions would really
need to match whether they are input or output ones.


Details on the implementation:

* The implementation is currently very C-ish (I even started it as pure
C). It's a GObject class, and it doesn't wrap a C++ class. I might
change that especially if you'd like me to. As the implementation
currently is, it might not be too useful regarding the complexity, as I
wouldn't have nothing to inherit from, and the bulk of the code is
implementing GObject interfaces which means a lot of C function pointers.
I'm however open to any design suggestions.

* Support for GTK 3.2 to 3.6 requires a fairly ugly hack. It's
documented, and there doesn't seem to be any other way to do it. It's
unrelated to Scintilla, but to GTK 3.2 having changed things and not
introduced new ones until 3.8. The only way not to require any hack
would be to re-implement the whole ATK interfaces inheritance currently
gives us, but I'd really rather not do that.
Apart that, GTK 2 and 3 is fully supported. GTK2 minimum version might
currently be 2.22, I'm not totally sure and didn't investigate too much yet.

* Most access to the data is currently made through
scintilla_send_message(). It might or might not be interesting to port
to something else. I must admit I'm not too familiar with those
internal parts, so I might be missing something obvious, suggestions
welcome.
Using at least sci->WndProc() might be interesting if the code should
become generic though.

* Implementation for detecting readonly state and associated document
changes is suboptimal, as there is no notifications for those (that I
know of, and at least not for associated document). Currently the value
is cached and checked in UPDATEUI.

* Similarly for selection and caret, because ATK wants to differentiate
caret moves from selection changes, but that can be done in
SC_UPDATE_SELECTION and is more specific to this, so probably not worth
considering a specific notification.


Regards,
Colomban

[Neil Hodgson]

Colomban:

> Here is an initial patch that give initial working support for at least
> the most naive part of accessibility.

Thanks.

> * It currently reports positions as Scintilla reports them (i.e. in
> bytes), while ATK expects character count. I don't yet have assessed
> the impact, but it's likely it should be fixed. It however might be
> slightly problematic when having to locate the Nth character in the
> buffer. Is there a cached way to query that, that don't mean reading
> the whole buffer from the start?

Each position query is commonly close to other recent queries so can be effectively managed with a single cached location. To get the feature working, just call SCI_POSITIONRELATIVE and SCI_COUNTCHARACTERS as needed.

Providing a character-based API over the byte-based document can be very costly in space and/or time. I’ve worked on this issue but the code was too complex for me to want to support and I can’t find it now.

An approach with reasonable space costs is to build a tree tiling over the document with each leaf storing the number of characters for a range of, say 100-200 bytes and with non-leaf nodes summing their child nodes. Document insertion/deletion invalidates/deletes just those tiles affected and their parent nodes. Within a tile, a linear search will be reasonably fast and can be omitted for pure ASCII nodes which are often common and where #bytes==#characters.

> * The implementation is currently very C-ish (I even started it as pure
> C). It's a GObject class, and it doesn't wrap a C++ class. I might
> change that especially if you'd like me to.

I’m much more interested in the functionality than in it being C++. Scintilla should really have been built with gtkmm but in the early days I thought it likely to not be maintained like so many language bindings.

> As the implementation
> currently is, it might not be too useful regarding the complexity, as I
> wouldn't have nothing to inherit from, and the bulk of the code is
> implementing GObject interfaces which means a lot of C function pointers.
> I'm however open to any design suggestions.
>
> * Support for GTK 3.2 to 3.6 requires a fairly ugly hack.

The #iffery is unpleasant but supporting old versions is worth the trouble.

> * Most access to the data is currently made through
> scintilla_send_message(). It might or might not be interesting to port
> to something else. I must admit I'm not too familiar with those
> internal parts, so I might be missing something obvious, suggestions
> welcome.
> Using at least sci->WndProc() might be interesting if the code should
> become generic though.

Calling ScintillaBase rather than ScintillaGTK is interesting. If I was doing this, I’d likely move functionality from ScintillaObjectAccessible into ScintillaGTK where it has simpler access to the private details of the classes and can call directly instead of going through scintilla_send_message/WndProc.

The way the target is saved and restored in, for example, “…cut_text”, doesn’t take into account that text was added or removed and so the target ends may need to move similar to Editor::MovePositionForInsertion/Deletion. Since there is already SCN_MODIFIED handling, there could be a “savedTarget” on the ScintillaObjectAccessible which is moved about as needed similar to how Editor maintains the selection and brace highlights.

> * Implementation for detecting readonly state and associated document
> changes is suboptimal, as there is no notifications for those (that I
> know of, and at least not for associated document). Currently the value
> is cached and checked in UPDATEUI.

SCI_SETREADONLY could have an implementation in ScintillaGTK that calls its base class and also nudges any attached ScintillaObjectAccessible.

Neil

[Colomban Wendling]

>> […] It however might be slightly problematic when having to locate

>> the Nth character in the buffer. Is there a cached way to query
>> that, that don't mean reading the whole buffer from the start?
>
> Each position query is commonly close to other recent queries so can
> be effectively managed with a single cached location. To get the
> feature working, just call SCI_POSITIONRELATIVE and
> SCI_COUNTCHARACTERS as needed.

That works when it is known the document didn't change, but the cache
has to be dropped anytime the contents changed.

But yeah in my case at least I can not worry about performance too much
for the moment and see later if it needs to be optimized.

> Providing a character-based API over the byte-based document can be
> very costly in space and/or time. I’ve worked on this issue but the
> code was too complex for me to want to support and I can’t find it
> now.
>
> An approach with reasonable space costs is to build a tree tiling
> over the document with each leaf storing the number of characters
> for a range of, say 100-200 bytes and with non-leaf nodes summing
> their child nodes. Document insertion/deletion invalidates/deletes
> just those tiles affected and their parent nodes. Within a tile, a
> linear search will be reasonably fast and can be omitted for pure
> ASCII nodes which are often common and where #bytes==#characters.

I was thinking about a simpler (well, I think) or naive implementation,
that would simply attach a character offset to each line, and then
compute from there. When a line changes, it invalidates the cached
offset on the following lines.
It can be made lazy by only computing the offsets when effectively
needed, not to impact too much code that don't query character offsets
(the "only" cost would be invalidating following lines if the current
one has a valid computed offset).
It's less efficient than a tree, and it's not very possible to improve
granularity, but OTOH it seems fairly easier to implement.

I didn't think on this too much, so maybe it's absurd at some level, but
that was a thought. It should be able to speed up COUNTCHARACTERS when
it spans lines. It could also speed up some use of POSITIONRELATIVE
spanning multiple lines, as the number of characters on a line would be
known.

Regards,
Colomban

[Colomban Wendling]

>> As the implementation currently is, it might not be too useful
>> regarding the complexity, as I wouldn't have nothing to inherit
>> from, and the bulk of the code is implementing GObject interfaces
>> which means a lot of C function pointers. I'm however open to any
>> design suggestions.
>>
>> * Support for GTK 3.2 to 3.6 requires a fairly ugly hack.
>
> The #iffery is unpleasant but supporting old versions is worth the
> trouble.

It's a little worse than #iffery alone (look at the #else part of
widget_get_accessible_impl), but yes, I think it's useful at least for
now. Well, it's arguable that supporting "old" GTK3 versions is not
very useful (unlike supporting some GTK2 versions, which is still
definitely in use by relevant applications), but still.

>> * Most access to the data is currently made through
>> scintilla_send_message(). It might or might not be interesting to
>> port to something else. I must admit I'm not too familiar with
>> those internal parts, so I might be missing something obvious,
>> suggestions welcome. Using at least sci->WndProc() might be
>> interesting if the code should become generic though.
>
> Calling ScintillaBase rather than ScintillaGTK is interesting.

I mostly did that to avoid having to expose all the guts of ScintillaGTK
so I could use it in another source file. It's not very pretty beside
that, and actually looks kind of odd when seen in a ScintillaGTK header.

> If I was doing this, I’d likely move functionality from
> ScintillaObjectAccessible into ScintillaGTK where it has simpler
> access to the private details of the classes and can call directly
> instead of going through scintilla_send_message/WndProc.

That might be a good idea indeed. However, I'd be slightly afraid of
the amount of code that this would add to ScintillaGTK.cxx that is
already about 3300 lines long. And I doubt spreading the implementation
of the class over more than one source is a great idea, is it?

But that might not be a real concern though, maybe 800-ish lines (and
maybe less, if accessing to some internals can shrink that down) is
reasonable enough. Or even all of the 1200ish if the bulk is already there.

> The way the target is saved and restored in, for example,
> “…cut_text”, doesn’t take into account that text was added or
> removed and so the target ends may need to move similar to
> Editor::MovePositionForInsertion/Deletion. Since there is already
> SCN_MODIFIED handling, there could be a “savedTarget” on the
> ScintillaObjectAccessible which is moved about as needed similar to
> how Editor maintains the selection and brace highlights.

Indeed, good point. BTW, is it a problem to mess up the target? By
that, I mean, is it guaranteed to be kept unchanged even between events?

Otherwise, maybe inserting/deleting from the buffer could be rather
performed using internal methods like InsertString(), like I did in
paste_received() -- because as paste is asynchronous, it is possible
(although probably unlikely) that the associated document changed by the
time the paste is actually received. Or at least that's what
GtkTextView's implementation suggests, I didn't actually check what
events can be dispatched while waiting for the clipboard.

>> * Implementation for detecting readonly state and associated
>> document changes is suboptimal, as there is no notifications for
>> those (that I know of, and at least not for associated document).
>> Currently the value is cached and checked in UPDATEUI.
>
> SCI_SETREADONLY could have an implementation in ScintillaGTK that
> calls its base class and also nudges any attached
> ScintillaObjectAccessible.

Indeed. Document change could be handled similarly by overriding
SetDocPointer() actually, as it is virtual.

Regards,
Colomban

[Neil Hodgson]

Colomban Wendling:

> I was thinking about a simpler (well, I think) or naive implementation,
> that would simply attach a character offset to each line, and then
> compute from there. When a line changes, it invalidates the cached
> offset on the following lines.

Its likely a reasonable approach for many files. I didn’t pursue per-line since it doesn’t work so well on long lines, there is a 32 or 64 bit per-line overhead and the overhead isn’t tuneable: a tree can be made more or less granular depending on application need.

> It can be made lazy by only computing the offsets when effectively
> needed, not to impact too much code that don't query character offsets
> (the "only" cost would be invalidating following lines if the current
> one has a valid computed offset).

In the other direction, it could use the same Partitioning class as is used for line starts to allow later lines to move as required. An insertion/deletion of whole characters on one line would add to/subtract from that line's end. Inserting character fragments or changing inside a character would lead to the line being remeasured in characters and the delta from the previous line character width applied. Multi-line changes would be more complex.

Due to the storage cost, adding a character count index should be a choice for the application. Does the accessibility support override that?

Another issue is that some platforms (notably Cocoa but .Net is another possibility) really prefer to count UTF-16 code units, not characters.

Neil

[Neil Hodgson]

Colomban Wendling:

> It's a little worse than #iffery alone (look at the #else part of
> widget_get_accessible_impl), but yes, I think it's useful at least for
> now. Well, it's arguable that supporting "old" GTK3 versions is not
> very useful (unlike supporting some GTK2 versions, which is still
> definitely in use by relevant applications), but still.

GTK+ 2 is nice and stable so I prefer it for SciTE.

We should probably have a discussion on the new versioning for GTK+ and how that affects Scintilla.
https://blog.gtk.org/2016/09/01/versioning-and-long-term-stability-promise-in-gtk/

The lack of stability within the development series means my preference would be to just target long term stable releases of GTK+ in the future. I expect others want new features more quickly though.

> That might be a good idea indeed. However, I'd be slightly afraid of
> the amount of code that this would add to ScintillaGTK.cxx that is
> already about 3300 lines long. And I doubt spreading the implementation
> of the class over more than one source is a great idea, is it?

The huge Godzilla classes in Scintilla like Editor are bad and I’d love to find better ways to subdivide.

Implementing a single class in multiple files wouldn’t be good.

Possibly have a friend class to ScintillaGTK that is responsible for accessibility.

> Indeed, good point. BTW, is it a problem to mess up the target? By
> that, I mean, is it guaranteed to be kept unchanged even between events?

Until now it hasn’t been an issue so its likely that some applications are depending on it not changing. Changing it for performing a “cut” operation initiated by the user is more reasonable than using it to retrieve text when performing an information retrieval.

That leads to the question of application control. The application can remove all intrinsic handling of “cut” from Scintilla (keyboard, context menu) and implement its own that may, for example, add different clipboard formats or disallow “cut" in some circumstances. If accessibility tunnels in and performs a cut, does the application get to veto this or, at least, discover that it has occurred?

Neil

[Colomban Wendling]

Le 28/09/2016 à 01:06, Neil Hodgson a écrit :
> […]

>
> We should probably have a discussion on the new versioning for GTK+
> and how that affects Scintilla.
> https://blog.gtk.org/2016/09/01/versioning-and-long-term-stability-promise-in-gtk/
>
> The lack of stability within the development series means my
> preference would be to just target long term stable releases of GTK+
> in the future. I expect others want new features more quickly
> though.

I don't think we can really make an informed decision right now, because
whether it's reasonable to support both stable and development versions
will probably depend mostly on how much backward compatibility code is
required.

However, it could possibly be reasonable to support stables versions,
plus the latest "development" one, which will in the end become another
supported stable version. That is, not try and support every
development releases, and people targeting development release have to
keep up. But again, that will probably depend on how much work it would
be to keep up, and the interest from various parties.

Regards,
Colomban

[Colomban Wendling]

Le 28/09/2016 à 01:06, Neil Hodgson a écrit :

> Colomban Wendling:

>
>> That might be a good idea indeed. However, I'd be slightly afraid
>> of the amount of code that this would add to ScintillaGTK.cxx that
>> is already about 3300 lines long. And I doubt spreading the
>> implementation of the class over more than one source is a great
>> idea, is it?
>
> The huge Godzilla classes in Scintilla like Editor are bad and I’d
> love to find better ways to subdivide.
>
> Implementing a single class in multiple files wouldn’t be good.
>
> Possibly have a friend class to ScintillaGTK that is responsible for
> accessibility.

That sounds like a pretty good idea.

Attached is a new early version of the patch, rewritten as a C++ class,
much like ScintillaGTK. It will allow to make it friend with
ScintillaGTK easily if/when needed -- and it might also be nice to fit
in a future generic accessibility layer.

Also, it fixes maintaining the target, and a few details, but not yet
much more new actual things.

If it looks a mostly reasonable approach, I'll work on improving it further.

>> Indeed, good point. BTW, is it a problem to mess up the target?
>> By that, I mean, is it guaranteed to be kept unchanged even between
>> events?
>
> Until now it hasn’t been an issue so its likely that some
> applications are depending on it not changing. Changing it for
> performing a “cut” operation initiated by the user is more reasonable
> than using it to retrieve text when performing an information
> retrieval.

Yes. Well, retrieving the text is easy to do without touching the
target, it's only insertion/deletion that plain API usage would suggest
using the target.

> That leads to the question of application control. The application
> can remove all intrinsic handling of “cut” from Scintilla (keyboard,
> context menu) and implement its own that may, for example, add
> different clipboard formats or disallow “cut" in some circumstances.
> If accessibility tunnels in and performs a cut, does the application
> get to veto this or, at least, discover that it has occurred?

That's a good question. ATK does have an EditableText interface [1],
that allows to control text modification, so I guess a well behaved
editable component will implement it.

[1] https://developer.gnome.org/atk/stable/AtkEditableText.html

An application could discover some unexpected changes by listening to
INSERTTEXT/DELETETEXT, or veto insertions in INSERTCHECK/DELETECHECK(?),
but not much more indeed. A solution could be to have additional
notifications that could be trapped, like CUT/PASTE or something.

Alternatively, a possibly ugly solution, but that would allow total
control over that, would be to expose enough of the accessible part so
applications could subclass it to alter how it handles requests in the
EditableText interface. But that's not very "scintillay", and probably
way too annoying for the application.


Regards,
Colomban

[Colomban Wendling]

Le 28/09/2016 à 00:42, Neil Hodgson a écrit :
> Colomban Wendling:
>
>> I was thinking about a simpler (well, I think) or naive
>> implementation, that would simply attach a character offset to each
>> line, and then compute from there. When a line changes, it
>> invalidates the cached offset on the following lines.
>
> Its likely a reasonable approach for many files. I didn’t pursue
> per-line since it doesn’t work so well on long lines, there is a 32
> or 64 bit per-line overhead and the overhead isn’t tuneable: a tree
> can be made more or less granular depending on application need.

Yeah totally, line-based is likely to work fine for most files, but has
poor granularity. And possibly would affect the line overhead in any case.

>> It can be made lazy by only computing the offsets when effectively
>> needed, not to impact too much code that don't query character
>> offsets (the "only" cost would be invalidating following lines if
>> the current one has a valid computed offset).
>
> In the other direction, it could use the same Partitioning class as
> is used for line starts to allow later lines to move as required. An
> insertion/deletion of whole characters on one line would add
> to/subtract from that line's end. Inserting character fragments or
> changing inside a character would lead to the line being remeasured
> in characters and the delta from the previous line character width
> applied. Multi-line changes would be more complex.
>
> Due to the storage cost, adding a character count index should be a
> choice for the application. Does the accessibility support override
> that?

When accessibility is enabled, I think it's reasonable to just use the
extra space, but indeed when it's not it shouldn't be forced upon the
application.

If it could be enabled/disabled dynamically, it would be possible for
the accessible part to ask for it when activated, and release it when no
longer needed.

> Another issue is that some platforms (notably Cocoa but .Net is
> another possibility) really prefer to count UTF-16 code units, not
> characters.

Err, good point. Some things want code units, some code points, and
some other full display characters. That does make it very hard to
provide something that would please everyone.

I'm not even totally certain what ATK wants. I would guess code points,
because it's common, and because it can retrieve a single character as a
Unicode value, and I don't imagine it expects combinations to be
applied, but maybe it does…

Do you think of any way to sort the needs of everyone out, or does this
suggest that it should be an internal detail left to each user?
I slightly fear many implementations that need to keep up with any
possible change in the buffer, but if they are all reasonably different,
maybe it doesn't make much sense to combine them either. Or maybe one
generic position tracker class (e.g. on top of Partitioning) that
listens on the document/editor, and leaves the task of actually filling
in the measurements to each subclass (obviously, current positions could
be one candidate). I don't know.


Regards,
Colomban

[Neil Hodgson]

Colomban Wendling:

> If it could be enabled/disabled dynamically, it would be possible for
> the accessible part to ask for it when activated, and release it when no
> longer needed.

Seems sensible.

> Do you think of any way to sort the needs of everyone out, or does this
> suggest that it should be an internal detail left to each user?

There should be an API to enable a single character to byte mapping with an enum containing BY_CHARACTER and NONE implemented initially. Later BY_CODE_UNIT or BY_CLUSTER could be implemented if wanted.

The application or binding could choose this. For example, a .Net binding may choose to always enable it and present a character-oriented API. Accessibility modes could also choose it overriding the application. The SCI_POSITIONRELATIVE, SCI_COUNTCHARACTERS and similar APIs would always be available but faster when the character to byte map matches.

Allowing both BY_CHARACTER and BY_CODE_UNIT maps at once would be more complex.

Neil

[Neil Hodgson]

Colomban Wendling:

> Yes. Well, retrieving the text is easy to do without touching the
> target, it's only insertion/deletion that plain API usage would suggest
> using the target.

It can be avoided for deletion by calling SCI_DELETERANGE.

> <1-Initial_accessibility_support_for_the_GTK_platform.patch>

GetCharacterCount is SCI_COUNTCHARACTERS(0, SCI_LENGTH)).

GetCharacterAtOffset is
target.start = SCI_POSITIONRELATIVE(0, offset)
target.end = SCI_POSITIONRELATIVE(target.start, 1)
sCharacter = SCI_TARGETASUTF8
call GTK+ to turn UTF-8 string into gunichar

Neil

[Colomban Wendling]

Hi,

Here is a new version using a lot more internals where it makes things
easier/not harder, and now the accessible is a friend of ScintillaGTK.

It also fixes:

* Now uses character position

* Now converts to and from UTF-8

* Abortion when ThisFromAccessible() fails (which should almost never
happen, unless maybe right when accessibility is being disabled). This
is just me not being familiar with C++ exceptions (and `throw` without
argument being weird).

* Accessible object destruction (I sometimes tried to emit signals in
finalize, which GObject really doesn't like), plus I forgot to chain up.

* Theoretically possible use-after-free in the Paste() code, as GTK
paste might it might be truly asynchronous (unlike what I supposed from
GtkTextViewAccessible I used as template originally)

* Don't re-define "GTK"


Paste code is fairly redundant with ScintillaGTK::Paste(), plus it
doesn't handle half of what ScintillaGTK::Paste() does, so it probably
should be replaced. However, it seemed non-trivial to allow specifying
a paste position to that code.
I made a naive attempt at adding a class member specifying the position,
but I'm quite afraid I would miss several places where it has to be
(re)set. But OTOH, passing in an argument seems fairly convoluted too,
so I'm not sure.
Do you have an opinion on that?

I also exported the ConvertText() function from ScintillaGTK, but it's
likely it should get a better name/prefix/scope now it's not static
anymore. Suggestions welcome.

Le 01/10/2016 à 01:28, Neil Hodgson a écrit :
> Colomban Wendling:
>
>> Yes. Well, retrieving the text is easy to do without touching the
>> target, it's only insertion/deletion that plain API usage would suggest
>> using the target.
>
> It can be avoided for deletion by calling SCI_DELETERANGE.

Yes, but wouldn't this also effectively alter the target, as the data it
points to has changed? Or will the target be moved as needed?
I mean, to keep the target pointing to reasonably the same positions, it
has to move accordingly if it's in or after the deleted range.

I now use pdoc->DeleteChars() directly, but it rises the same question.

>> <1-Initial_accessibility_support_for_the_GTK_platform.patch>
>
> GetCharacterCount is SCI_COUNTCHARACTERS(0, SCI_LENGTH)).
>
> GetCharacterAtOffset is
> target.start = SCI_POSITIONRELATIVE(0, offset)
> target.end = SCI_POSITIONRELATIVE(target.start, 1)
> sCharacter = SCI_TARGETASUTF8
> call GTK+ to turn UTF-8 string into gunichar

Yes, I basically did that (using internal API and some manual things to
avoid extra copy in the conversion code). I didn't optimize position
computation farther than when I have 2 positions representing a range,
so it's likely to be slow-ish on large files for the moment.

Regards,
Colomban

[Neil Hodgson]

Colomban:

> Here is a new version using a lot more internals where it makes things
> easier/not harder, and now the accessible is a friend of ScintillaGTK.

Its looking quite reasonable although I haven’t read it that closely yet.

“using namespace” in headers has always ended up biting me. Placing ScintillaGTK inside a “namespace Scintilla {” (similar to most headers in src like Indicator.h) achieves mostly the same thing without exposing all of the Scintilla namespace just for including ScintillaGTK.cxx. Much of the point of the Scintilla namespace is so that we can avoid clashes with platform names if its ever needed as it was on other platforms.

It looks like accessibility is a manageable amount of code if this is near its complete extent.

> * Theoretically possible use-after-free in the Paste() code, as GTK
> paste might it might be truly asynchronous (unlike what I supposed from
> GtkTextViewAccessible I used as template originally)

Asynchronous clipboard is such a pain. I don’t much like allowing a Document lifetime to depend on the asynchronous arrival and possibly this should be tied to the widget (ScintillaGTK or ScintillaGTKAccessible). If you are trying to cope with a document change while waiting for the response then its unlikely and will be unclear with the text ‘appearing’ in an inactive document. The widget is needed to set the selection and scroll into view. The ScintillaGTK or ScintillaGTKAccessible could have their refcount increased while waiting for the response and the response ignored if it arrives at an otherwise closed object or where it has a different pdoc.

> Paste code is fairly redundant with ScintillaGTK::Paste(), plus it
> doesn't handle half of what ScintillaGTK::Paste() does, so it probably
> should be replaced. However, it seemed non-trivial to allow specifying
> a paste position to that code.

Its really built around the richer selection possibilities which aren’t of interest to the accessible version with a single position argument. Multiple selection is being ignored as is virtual space. There won’t be a rectangular marker or line paste marker when the paste data is coming from accessibility. Its providing the location, so the paste shouldn't delete the currently selected text. So the core is: ConvertEncoding; TransformLineEnds; InsertString; SetSelection; EnsureCaretVisible.

Are there hints about whether an accessible paste is used for hidden changes or is it intended to be like the user performing a paste so is allowed to set the selection to the position before performing it?

Orca will read stuff for me (yay!) but it doesn’t have a UI to exercise commands like cut and paste.

> I also exported the ConvertText() function from ScintillaGTK, but it's
> likely it should get a better name/prefix/scope now it's not static
> anymore. Suggestions welcome.

Putting it in the Scintilla namespace should be fine. Or make it a static method on ScintillaGTK.

Neil

[Colomban Wendling]

Le 04/10/2016 à 06:32, Neil Hodgson a écrit :
>> * Theoretically possible use-after-free in the Paste() code, as
>> GTK paste might it might be truly asynchronous (unlike what I
>> supposed from GtkTextViewAccessible I used as template originally)
>
> Asynchronous clipboard is such a pain. I don’t much like allowing a
> Document lifetime to depend on the asynchronous arrival and possibly
> this should be tied to the widget (ScintillaGTK or
> ScintillaGTKAccessible). If you are trying to cope with a document
> change while waiting for the response then its unlikely and will be
> unclear with the text ‘appearing’ in an inactive document. The widget
> is needed to set the selection and scroll into view. The ScintillaGTK
> or ScintillaGTKAccessible could have their refcount increased while
> waiting for the response and the response ignored if it arrives at an
> otherwise closed object or where it has a different pdoc.

I just wanted to "do the right thing", but yeah it's questionable what
it is.

The problem with adding a reference to ScintillaGTK is that it could
prevent the app from really destroying the widget, which isn't much
better than keeping the Document around.

A better approach is to hold a weak reference and drop the paste
response if the widget has been destroyed by then. Patch attached fixes
this in ScintillaGTK. It adds 51 lines for a theoretical issue, but it
might be worth sorting it out before it comes biting us.

I didn't handle Document changes, but it would be trivial to simply
compare pointers or something, and assume if it's the same pointer it's
the same Document.

The implementation is slightly more split that really needed (the
GObjectWatcher class), but it will make it easy to reuse for the same
thing in the accessible part.

Regards,
Colomban

[Colomban Wendling]

Le 04/10/2016 à 06:32, Neil Hodgson a écrit :

> Colomban:
>
>> Here is a new version using a lot more internals where it makes
>> things easier/not harder, and now the accessible is a friend of
>> ScintillaGTK.
>
> Its looking quite reasonable although I haven’t read it that closely
> yet.
>
> “using namespace” in headers has always ended up biting me. Placing
> ScintillaGTK inside a “namespace Scintilla {” (similar to most
> headers in src like Indicator.h) achieves mostly the same thing
> without exposing all of the Scintilla namespace just for including
> ScintillaGTK.cxx. Much of the point of the Scintilla namespace is so
> that we can avoid clashes with platform names if its ever needed as
> it was on other platforms.

Okay, fixed.

> It looks like accessibility is a manageable amount of code if this is
> near its complete extent.

Well, with my current knowledge, yes, I think it's close to being
complete feature-set-wise. It basically implements the same set of
interfaces GtkTextView does, so I'm fairly confident it's not too bad.

Of course, it's highly likely to have many implementation bugs, but
fixing those shouldn't change LOC count too much.

But this will be better asserted when some real accessibility users make
some tests and reports. I'm learning most of the user side of things as
I go.

>> Paste code is fairly redundant with ScintillaGTK::Paste(), plus it
>> doesn't handle half of what ScintillaGTK::Paste() does, so it
>> probably should be replaced. However, it seemed non-trivial to
>> allow specifying a paste position to that code.
>
> Its really built around the richer selection possibilities which
> aren’t of interest to the accessible version with a single position
> argument. Multiple selection is being ignored as is virtual space.
> There won’t be a rectangular marker or line paste marker when the
> paste data is coming from accessibility. Its providing the location,
> so the paste shouldn't delete the currently selected text. So the
> core is: ConvertEncoding; TransformLineEnds; InsertString;
> SetSelection; EnsureCaretVisible.

Makes sense.

I currently use the simpler GTK API gtk_clipboard_request_text() so the
encoding conversion on the input data is implicit -- it might be
converting back and forth, but well.
I added TransformLineEnds() which I didn't know about, thanks for the
hint. So it now respects SETPASTECONVERTENDINGS.

I'm not sure whether I should set the selection or scroll in response to
this however. GtkTextViewAccessible's code doesn't seem to, and the ATK
documentation doesn't say.

> Are there hints about whether an accessible paste is used for hidden
> changes or is it intended to be like the user performing a paste so
> is allowed to set the selection to the position before performing
> it?

I have no idea actually. I expect it'd be used to really perform a
"user" paste as there is also API for inserting arbitrary text, but it's
only a guess.

> Orca will read stuff for me (yay!) but it doesn’t have a UI to
> exercise commands like cut and paste.

No indeed, I'll have to investigate how and when those are used.
Possibly with totally different accessibility tools like voice inputs or
something, not sure.

>> I also exported the ConvertText() function from ScintillaGTK, but
>> it's likely it should get a better name/prefix/scope now it's not
>> static anymore. Suggestions welcome.
>
> Putting it in the Scintilla namespace should be fine. Or make it a
> static method on ScintillaGTK.

Okay, it's inside the Scintilla namespace now.


Attached is a v4 of the patch. Changes:

* Make ScintillaGTK notify ScintillaGTKAccessible of Document and
readonly state changes. This avoids having to continuously check the
values in the notify callback, so it's both cleaner and slightly faster,
which might be worth as the notify callback is called fairly often.
Downside is that ScintillaGTK is more aware of accessibility now, as it
has to forward some notifications.

* No more "using namespace" in the headers, and their contents is inside
the Scintilla namespace.

* The ScintillaGTKAccessible class is now in its header, and then some
method bodies have moved out of the class definition.

* Fix an invalid memory access in character extent computation due to a
missing C string terminator (oops)

* Paste code doesn't add a reference to the Document anymore, but
currently then has the theoretically possible "race" issue ScintillaGTK
currently has. See the other mail for a possible fix that once agreed
upon can be applied here too.

* Fix notifying ATK of readonly state change when switching Documents.


Also, I'm not quite sure if it's me (I don't think I did anything
relevant to that in the code) or an Orca update or something, but now
Orca mostly behaves when switching buffers (e.g. in SciTE), yay.

Regards,
Colomban

[Neil Hodgson]

Colomban Wendling:

> A better approach is to hold a weak reference and drop the paste
> response if the widget has been destroyed by then. Patch attached fixes
> this in ScintillaGTK. It adds 51 lines for a theoretical issue, but it
> might be worth sorting it out before it comes biting us.

OK. I’ll apply this after 3.7.0.

It may be difficult to be sure of correctness without a deliberately slow clipboard provider to test with.

Neil

[Neil Hodgson]

Colomban Wendling:

>> Much of the point of the Scintilla namespace is so
>> that we can avoid clashes with platform names if its ever needed as
>> it was on other platforms.
> Okay, fixed.

Scintilla on GTK+ doesn’t currently work with -DSCI_NAMESPACE because of a use of SCNotification in ScintillaWidget.h but this can be fixed with an extra qualification ‘Scintilla::’ when #if defined(__cplusplus) && defined (SCI_NAMESPACE) which I’ll add after 3.7.0.

>> It looks like accessibility is a manageable amount of code if this is
>> near its complete extent.
>
> Well, with my current knowledge, yes, I think it's close to being
> complete feature-set-wise. It basically implements the same set of
> interfaces GtkTextView does, so I'm fairly confident it's not too bad.

One issue that hasn’t been addressed yet is non-changeable text - ranges with a style that has the changeable or visible attribute false. ScintillaGTKAccessible goes straight to the Document and doesn’t check RangeContainsProtected. Using a style for changeable hasn’t worked well and its likely it will be augmented with an indicator but that would also be checked through RangeContainsProtected. The calls that may be affected are DeleteText and CutText.

There is some use of nullptr in the changed code which is C++11. It a reasonably safe feature on GTK+ as it was available in GCC from 4.6 but may have to be reverted back to NULL if this stops anyone building. It would be more of a problem in platform-independent or Win32 code as MSVC 2008 is still supported.
http://en.cppreference.com/w/cpp/compiler_support

Updated patch builds and works for me.

Neil

[Colomban Wendling]

Le 11/10/2016 à 00:05, Neil Hodgson a écrit :
> Colomban Wendling:
>
>>> Much of the point of the Scintilla namespace is so that we can
>>> avoid clashes with platform names if its ever needed as it was on
>>> other platforms.
>> Okay, fixed.
>
> Scintilla on GTK+ doesn’t currently work with -DSCI_NAMESPACE because
> of a use of SCNotification in ScintillaWidget.h but this can be fixed
> with an extra qualification ‘Scintilla::’ when #if
> defined(__cplusplus) && defined (SCI_NAMESPACE) which I’ll add after
> 3.7.0.

Okay. I just tried building with SCI_NAMESPACE, and my patch adds some
issues. One is easy, the other I'm not quite sure what it means:

> ../ScintillaGTK.cxx: In static member function ‘static void Scintilla::ScintillaGTK::Realize(GtkWidget*)’:
> ../ScintillaGTK.cxx:309:52: error: call of overloaded ‘ScintillaFromWidget(GtkWidget*&)’ is ambiguous
> ScintillaGTK *sciThis = ScintillaFromWidget(widget);
> ^
> In file included from ../ScintillaGTK.cxx:75:0:
> ../ScintillaGTK.h:249:15: note: candidate: Scintilla::ScintillaGTK* Scintilla::ScintillaFromWidget(GtkWidget*)
> ScintillaGTK *ScintillaFromWidget(GtkWidget *widget);
> ^~~~~~~~~~~~~~~~~~~
> ../ScintillaGTK.cxx:160:15: note: candidate: Scintilla::ScintillaGTK* ScintillaFromWidget(GtkWidget*)
> ScintillaGTK *ScintillaFromWidget(GtkWidget *widget) {
> ^~~~~~~~~~~~~~~~~~~

Okay it's because ScintillaFromWidget() definition in the .cxx isn't in
the namespace, but I don't see why it's a problem? Moreover as
ConvertText() doesn't show the issue.

Anyway, moving the function to a static method
ScintillaGTK::FromWidget() seems nice and resolves the issue. Should I
do that?

>>> It looks like accessibility is a manageable amount of code if
>>> this is near its complete extent.
>>
>> Well, with my current knowledge, yes, I think it's close to being
>> complete feature-set-wise. It basically implements the same set
>> of interfaces GtkTextView does, so I'm fairly confident it's not
>> too bad.
>
> One issue that hasn’t been addressed yet is non-changeable text -
> ranges with a style that has the changeable or visible attribute
> false. ScintillaGTKAccessible goes straight to the Document and
> doesn’t check RangeContainsProtected. Using a style for changeable
> hasn’t worked well and its likely it will be augmented with an
> indicator but that would also be checked through
> RangeContainsProtected. The calls that may be affected are DeleteText
> and CutText.

Fixed (in theory at least, I didn't yet effectively test with protected
ranges). As currently I implemented CutText() by calling DeleteText, I
had to change it only in one place.

> There is some use of nullptr in the changed code which is C++11. It a
> reasonably safe feature on GTK+ as it was available in GCC from 4.6
> but may have to be reverted back to NULL if this stops anyone
> building. It would be more of a problem in platform-independent or
> Win32 code as MSVC 2008 is still supported.
> http://en.cppreference.com/w/cpp/compiler_support

Okay. Actually, I replaced all `nullptr` with `0` (as IIUC it's a valid
NULL value in C++, right?), and removed the in-class non-static member
initialization so that it builds without extra options on GCC 4.9 (which
doesn't enable C++11 by default).

Attached is a v5 of the patch, changelog is:

* Don't use C++ features not enabled by default on GCC 4.9 (nullptr,
in-class non-static member initialization)

* Fix support of ATK < 2.16 (tested on 2.14)

* Honour protected ranges


Regards,
Colomban

[Neil Hodgson]

Colomban Wendling:

> Okay. I just tried building with SCI_NAMESPACE, and my patch adds some
> issues. One is easy, the other I'm not quite sure what it means:
>
>> ../ScintillaGTK.cxx: In static member function ‘static void Scintilla::ScintillaGTK::Realize(GtkWidget*)’:
>> ../ScintillaGTK.cxx:309:52: error: call of overloaded ‘ScintillaFromWidget(GtkWidget*&)’ is ambiguous
>> ScintillaGTK *sciThis = ScintillaFromWidget(widget);

…

> Okay it's because ScintillaFromWidget() definition in the .cxx isn't in
> the namespace, but I don't see why it's a problem? Moreover as
> ConvertText() doesn't show the issue.

ConvertText shows a link-time issue for me with SciTE as does scintilla_object_accessible_widget_get_accessible_impl.

> Anyway, moving the function to a static method
> ScintillaGTK::FromWidget() seems nice and resolves the issue. Should I
> do that?

That seems reasonable. For ConvertText and scintilla_object_accessible_widget_get_accessible_impl, they can either be surrounded by an explicit conditional namespace or become static methods.

With added namespace declarations, this builds and runs for me both with and without SCI_NAMESPACE.

> Fixed (in theory at least, I didn't yet effectively test with protected
> ranges). As currently I implemented CutText() by calling DeleteText, I
> had to change it only in one place.

OK.

> Okay. Actually, I replaced all `nullptr` with `0` (as IIUC it's a valid
> NULL value in C++, right?),

Yes.

> and removed the in-class non-static member
> initialization so that it builds without extra options on GCC 4.9 (which
> doesn't enable C++11 by default).

Good.

> <1-Initial_accessibility_support_for_the_GTK_platform_v5.patch>

There are some repeated (Scintilla.h, ScintillaWidget.h) and unnecessary includes. unistd.h implies POSIX and not Win32 so should be avoided (even with a HAVE* test as it can hide implicit uses) unless this is only destined for POSIX. sys/types.h is also a bit too platform-specific and some types it defines (like size_t) should come from their ‘home’ header stddef.h. The includes aren’t in the standard order defined by scripts/HeaderOrder.txt which makes it more difficult to check.

Attached is an updated HeaderOrder.txt with the new headers needed for accessibility added along with the HeaderCheck.py script I use to check. The script belongs in a sibling directory to scintilla to read the files correctly and hasn’t been published with Scintilla as its output can be confusing. Also, a changed set of #includes for ScintillaGTKAccessible.cxx.

Neil

[Colomban Wendling]

On 10/10/2016 23:32, Neil Hodgson wrote:
> Colomban Wendling:
>
>> A better approach is to hold a weak reference and drop the paste
>> response if the widget has been destroyed by then. Patch attached
>> fixes this in ScintillaGTK. It adds 51 lines for a theoretical
>> issue, but it might be worth sorting it out before it comes biting
>> us.
>
> OK. I’ll apply this after 3.7.0.

Attached is an updated version that replaces nullptr use with 0. Builds
fine under GCC 4.9.

> It may be difficult to be sure of correctness without a deliberately
> slow clipboard provider to test with.

Actually it can be tricked fairly easily: see attached test program that
destroys the widget directly after sending a PASTE command: if the paste
really is asynchronous, no matter how fast, it'll try and access a
destroyed ScintillaGTK. And indeed, without the patch it crashes, and
with it it works fine (and Valgrind is also happy).

To test, just place something in the clipboard and launch the test
program. I verified it really was asynchronous the good old printf way,
but watching for ScintillaGTK::Paste() and
ScintillaGTK::ReceivedSelection() in a debugger would do too -- or
simply breaking on ScintillaGTK::ReceivedSelection() and checking the
stack trace.


Regards,
Colomban

[Colomban Wendling]

Le 12/10/2016 à 01:21, Neil Hodgson a écrit :
> […]

>> Okay it's because ScintillaFromWidget() definition in the .cxx
>> isn't in the namespace, but I don't see why it's a problem?
>> Moreover as ConvertText() doesn't show the issue.
>
> ConvertText shows a link-time issue for me with SciTE as does
> scintilla_object_accessible_widget_get_accessible_impl.

Yeah OK that's expected then. Somehow I couldn't build SciTE with
SCI_NAMESPACE, because it seems to use unqualified Sci_RangeToFormat in
SciTEGTK.cxx.

>> Anyway, moving the function to a static method
>> ScintillaGTK::FromWidget() seems nice and resolves the issue.
>> Should I do that?
>
> That seems reasonable. For ConvertText and
> scintilla_object_accessible_widget_get_accessible_impl, they can
> either be surrounded by an explicit conditional namespace or become
> static methods.
>
> With added namespace declarations, this builds and runs for me both
> with and without SCI_NAMESPACE.

I added the namespace around ConvertText() definition, and moved the two
others as static methods. Now it builds and links fine, given the
slight modification to ScintillaWidget.h you mentioned.

>> <1-Initial_accessibility_support_for_the_GTK_platform_v5.patch>
>
> There are some repeated (Scintilla.h, ScintillaWidget.h) and
> unnecessary includes. unistd.h implies POSIX and not Win32 so should
> be avoided (even with a HAVE* test as it can hide implicit uses)
> unless this is only destined for POSIX. sys/types.h is also a bit too
> platform-specific and some types it defines (like size_t) should come
> from their ‘home’ header stddef.h. The includes aren’t in the
> standard order defined by scripts/HeaderOrder.txt which makes it more
> difficult to check.
>
> Attached is an updated HeaderOrder.txt with the new headers needed
> for accessibility added along with the HeaderCheck.py script I use to
> check. The script belongs in a sibling directory to scintilla to read
> the files correctly and hasn’t been published with Scintilla as its
> output can be confusing. Also, a changed set of #includes for
> ScintillaGTKAccessible.cxx.

Okay. I must admit that I mostly dropped a copy of what there is in
ScintillaGTK out of frustration trying to include only the headers I
knew I needed, so yeah I'm not surprised it was a bit of a mess. And I
didn't know about HeaderOrder.txt, so that's a plus now.
Thanks for sorting this out, I would likely have forgot.

I used your includes, but dropped glib-object.h and glib/gstdio.h,
because the first isn't really needed if you include gtk/gtk.h, and I
have no idea why I had the second -- and neither of those were used
anywhere else in Scintilla before.

So, updated patch attached:

* Fix includes (your changes basically)
* Fix SCI_NAMESPACE support


Regards,
Colomban

[Colomban Wendling]

Le 12/10/2016 à 16:02, Colomban Wendling a écrit :
> […]
>
> So, updated patch attached:

Well, now it is. Oops.

[Neil Hodgson]

Colomban Wendling:

> Actually it can be tricked fairly easily: see attached test program that
> destroys the widget directly after sending a PASTE command:

Demonstrated OK so looks like a good change.

Neil

[Neil Hodgson]

Colomban Wendling:

> Yeah OK that's expected then. Somehow I couldn't build SciTE with
> SCI_NAMESPACE, because it seems to use unqualified Sci_RangeToFormat in
> SciTEGTK.cxx.

Unlike most libraries, the namespace in Scintilla was intended for internal use instead of by clients with Scintilla providing a C interface. The namespace does leak out though.

> I added the namespace around ConvertText() definition, and moved the two
> others as static methods. Now it builds and links fine, given the
> slight modification to ScintillaWidget.h you mentioned.

OK. You also qualified FromWidget calls with ScintillaGTK:: which doesn’t appear needed although I don’t really care either way.

If you think it is OK, this appears safe enough to be committed a couple of days after 3.7.0 (there is a pause in case something fatal is released). While an accessibility tool could poke the code in an unexpected and fatal way, we’re unlikely to find out about this without making the code available widely.

Neil

[Colomban Wendling]

Le 13/10/2016 à 06:51, Neil Hodgson a écrit :
> Colomban Wendling:

>> I added the namespace around ConvertText() definition, and moved
>> the two others as static methods. Now it builds and links fine,
>> given the slight modification to ScintillaWidget.h you mentioned.
>
> OK. You also qualified FromWidget calls with ScintillaGTK:: which
> doesn’t appear needed although I don’t really care either way.

Yes, it could be unqualified in many places, but I find FromWidget() a
little unclear when not qualified with anything, while
ScintillaGTK::FromWidget() seems unambiguous to the reader.
But that's probably just me not being used enough to C++isms, I can
unqualify it where unnecessary if you'd like.

> If you think it is OK, this appears safe enough to be committed a
> couple of days after 3.7.0 (there is a pause in case something fatal
> is released).

I'd like to apply the fix for the asynchronous paste, so ideally wait
for the other patch to get merged and patch over that. I can also merge
the two if you'd like, but I find it less clean.
Apart from that, I'll review it again but I think it's fairly OK as it
is now. And it definitely should have no negative impact when
accessibility isn't enabled, so I'd think it's safe, and only improves
the situation for when a11y is enabled.

> While an accessibility tool could poke the code in an
> unexpected and fatal way, we’re unlikely to find out about this
> without making the code available widely.

Agreed, at this point I think we need testing more than anything else.


Regards,
Colomban

[Neil Hodgson]

Colomban Wendling:

> Yes, it could be unqualified in many places, but I find FromWidget() a
> little unclear when not qualified with anything, while
> ScintillaGTK::FromWidget() seems unambiguous to the reader.

I tend to treat both static and instance methods the same. Some people even like qualifying instance methods.

> I'd like to apply the fix for the asynchronous paste, so ideally wait
> for the other patch to get merged and patch over that.

OK

Neil

[Neil Hodgson]

Colomban Wendling:

> <1-GTK__Avoid_theoretical_access_to_a_destroyed_object_on_async_paste.patch><pasterace.cpp>

Committed
https://sourceforge.net/p/scintilla/code/ci/96471898fda576bad106c580c695e9474ce82ba6/

Neil

[Neil Hodgson]

Colomban Wendling:

> Well, now it is. Oops.

> …
> <1-Initial_accessibility_support_for_the_GTK_platform_v6.patch>

With the ‘theoretical’ patch applied, this one no longer applies cleanly.

Neil

[Colomban Wendling]

Le 20/10/2016 à 00:38, Neil Hodgson a écrit :
> Colomban Wendling:
>

>> <1-Initial_accessibility_support_for_the_GTK_platform_v6.patch>
>
> With the ‘theoretical’ patch applied, this one no longer applies cleanly.

Indeed, as it changed the class and the accessibility patch moves it.

Updated patch attached:

* Applies on current HG
* Avoid theoretical crash in accessible paste code
* Remove unnecessary qualifications of ScintillaGTK::FromWidget() calls
* Fix SetCaretOffset() implementation (use GOTOPOS, not SETCURRENTPOS)
* Fix GetCharacterExtents() at the last position (noticed mimicking a
click through the accessibility layer)

Regards,
Colomban

[Neil Hodgson]

Colomban Wendling:

> Updated patch attached:
>
> * Applies on current HG
> * Avoid theoretical crash in accessible paste code
> * Remove unnecessary qualifications of ScintillaGTK::FromWidget() calls
> * Fix SetCaretOffset() implementation (use GOTOPOS, not SETCURRENTPOS)
> * Fix GetCharacterExtents() at the last position (noticed mimicking a
> click through the accessibility layer)

OK, committed.
https://sourceforge.net/p/scintilla/code/ci/a08e0deb28b2babbfa572f18d8708f1e149d8e89/

Something that troubles me just a little is that ScintillaGtkAccessible.h exposes much of the implementation of ScintillaGtkAccessible to ScintillaGtk even though there is a relatively narrow interface from ScintillaGtk to ScintillaGtkAccessible - just NotifyReadOnly and ChangeDocument if I’m reading it correctly.

Since there is a pointer cycle (ScintillaGtk::accessible -> ScintillaGtkAccessible; ScintillaGtkAccessible::sci -> ScintillaGtk it may be worthwhile clarifying ownership / lifetime. That is, ScintillaGtk::accessible is the primary owning reference to ScintillaGtkAccessible and ScintillaGtkAccessible::sci is a ‘weak’ back pointer with no refcount.

Neil

[Neil Hodgson]

In the Universal Access settings pane, there is a “Large Text" option which appears to change the screen’s text scaling. When this is toggled, Scintilla redraws with larger/smaller fonts but uses old cached positioning data. In SciTE, you can force a reset of these by switching to another document and back but in Geany, (probably because of a different document/view strategy) the old positioning remains.

If there is an event sent when the user toggles “Large Text” then Scintilla could discard its cached data cleanly. While it could hold the previous text scale and detect a change at the start of drawing, I have tried this on other platforms and it can be unpleasantly messy.

Neil

[Neil Hodgson]

There are problems building this on Win32.

To support some clipboard behaviour, there is a Win32 CLIPFORMAT inside ScintillaGTK with a PLAT_GTK_WIN32 #if. To see CLIPFORMAT, GtkAccessible.cxx needs to #include <windows.h> in the same manner and position as ScintillaGTK.cxx.

I have been using a copy of GTK+ 2.20 for checking that Scintilla for GTK+ still builds on Windows but it contains an old Atk 1.x so doesn’t have ATK_CHECK_VERSION. Couldn’t find a good package of GTK+ for Windows. A GTK+ 3.6 didn’t work well. A 2.24 from PyGTK is better but still has ATK 1.x. It can all be made to compile by surrounding each #if ATK_CHECK_VERSION with #if defined(ATK_CHECK_VERSION) on an extra line but its ugly:

#if defined(ATK_CHECK_VERSION)
#if ATK_CHECK_VERSION(2, 10, 0)
iface->get_string_at_offset = GetStringAtOffset;
#endif
#endif

If ATK simply isn’t supported on Win32 then maybe #if all the accessibility with #if !defined(PLAT_GTK_WIN32) ?

Neil

[Matthew Brush]

On 2016-10-23 04:49 PM, Neil Hodgson wrote:
> Couldn’t find a good package of GTK+ for Windows.

Msys2[0] has recent GTK+ 2 & 3 packages.

Regards,
Matthew Brush

[0]: http://msys2.github.io/

[Colomban Wendling]

Le 24/10/2016 à 01:49, Neil Hodgson a écrit :
> There are problems building this on Win32.

Oops, indeed I didn't try that yet.

> To support some clipboard behaviour, there is a Win32 CLIPFORMAT
> inside ScintillaGTK with a PLAT_GTK_WIN32 #if. To see CLIPFORMAT,
> GtkAccessible.cxx needs to #include <windows.h> in the same manner
> and position as ScintillaGTK.cxx.

OK, that sounds easy enough.

> I have been using a copy of GTK+ 2.20 for checking that Scintilla for
> GTK+ still builds on Windows but it contains an old Atk 1.x so
> doesn’t have ATK_CHECK_VERSION. Couldn’t find a good package of GTK+
> for Windows. A GTK+ 3.6 didn’t work well. A 2.24 from PyGTK is better
> but still has ATK 1.x. It can all be made to compile by surrounding
> each #if ATK_CHECK_VERSION with #if defined(ATK_CHECK_VERSION) on an
> extra line but its ugly:
>
> #if defined(ATK_CHECK_VERSION)
> #if ATK_CHECK_VERSION(2, 10, 0)
> iface->get_string_at_offset = GetStringAtOffset;
> #endif
> #endif

Bummer. Okay, I see 2 solutions:

1. like yours, but just define ATK_CHECK_VERSION() if it's missing, so
there is a single place where that compatibility is handled.

#ifndef ATK_CHECK_VERSION
# define ATK_CHECK_VERSION(x, y, z) 0
#endif

2. drop the checks altogether and don't use that newer API. It's
probably not great, but e.g. GtkTextViewAccessible as of today doesn't
implement get_string_at_offset() nor use ATK_STATE_READ_ONLY, which are
the 2 newer things I'm checking for.
It's a bit sad I guess because it's what ATK's docs say one should use
now, but OTOH it doesn't seem to add so much functionality and
Accessibility Tools probably will keep and try to support the older apps
that don't implement those -- and e.g. implementing
get_string_at_offset() adds surface to be tested.


Are these the only problematic parts? Or am I using a lot of ATK 2.x stuff?

> If ATK simply isn’t supported on Win32 then maybe #if all the
> accessibility with #if !defined(PLAT_GTK_WIN32) ?

That might be an option too, and would have to be investigated. But I
would hope ATK provide some level of accessibility on Windows too.


Regards,
Colomban

[Neil Hodgson]

Matthew Brush:

> Msys2[0] has recent GTK+ 2 & 3 packages.

That configuration appears to the code to be POSIX, not Win32, so __WIN32__ isn’t defined so the Win32-specific features aren’t enabled.

With a minor change and some code disabled, SciTE builds and starts under Msys but hangs with any mouse click.

Defining __WIN32__ leads to a undefined reference between ScintillaGTK and ScintillaBase::WndProc. The gtk2 package includes ATK 2.22.0.

Scintilla for GTK+ on Win32 normally enables rectangular clipboard format compatible with Visual Studio, Alt as the default rectangular selection key, a wheel scrolling change, clipboard newline normalization, and an IME tweak.

Neil

[Neil Hodgson]

Colomban Wendling:

> Bummer. Okay, I see 2 solutions:
>
> 1. like yours, but just define ATK_CHECK_VERSION() if it's missing, so
> there is a single place where that compatibility is handled.
>
> #ifndef ATK_CHECK_VERSION
> # define ATK_CHECK_VERSION(x, y, z) 0
> #endif

OK.

> 2. drop the checks altogether and don't use that newer API. It's
> probably not great, but e.g. GtkTextViewAccessible as of today doesn't
> implement get_string_at_offset() nor use ATK_STATE_READ_ONLY, which are
> the 2 newer things I'm checking for.

I expect there is very little use of GTK+ on Windows compared to Unix so would not want to see poorer support on Unix just to help on Windows. Not having the richer interface may discourage implementers of accessibility tools.

> Are these the only problematic parts? Or am I using a lot of ATK 2.x stuff?

I have only really looked at build-time issues so don’t know what will happen at run time yet.

Neil

[Matthew Brush]

On 2016-10-24 03:11 PM, Neil Hodgson wrote:
> Matthew Brush:
>
>> Msys2[0] has recent GTK+ 2 & 3 packages.
>
> That configuration appears to the code to be POSIX, not Win32, so
__WIN32__ isn’t defined so the Win32-specific features aren’t enabled.
>

They're compiled with GCC rather than MSVC, but still for Windows using
Win32 headers, C ABI and runtime. The usual _WIN32 should be defined
(and G_OS_WIN32 from GLib). Not sure about __WIN32__, that appears to be
something defined by Borland C++?

Geany is using these libraries on Windows now as they're what GTK+
recommends since they stopped providing binary bundles on their website,
which is why I mentioned.

Regards,
Matthew Brush

[Neil Hodgson]

Matthew Brush:

> They're compiled with GCC rather than MSVC, but still for Windows using Win32 headers, C ABI and runtime. The usual _WIN32 should be defined (and G_OS_WIN32 from GLib). Not sure about __WIN32__, that appears to be something defined by Borland C++?

__WIN32__ was added for MinGW GCC for Geany according to this thread:
https://groups.google.com/d/topic/scintilla-interest/ymSExGxkYsM/discussion

> Geany is using these libraries on Windows now as they're what GTK+ recommends since they stopped providing binary bundles on their website, which is why I mentioned.

Does Geany correctly paste rectangular selections copied from Visual Studio or SciTE as rectangular now?

Neil

[Matthew Brush]

On 2016-10-24 06:58 PM, Neil Hodgson wrote:
> Matthew Brush:
>
>> They're compiled with GCC rather than MSVC, but still for Windows using Win32 headers, C ABI and runtime. The usual _WIN32 should be defined (and G_OS_WIN32 from GLib). Not sure about __WIN32__, that appears to be something defined by Borland C++?
>
> __WIN32__ was added for MinGW GCC for Geany according to this thread:
> https://groups.google.com/d/topic/scintilla-interest/ymSExGxkYsM/discussion
>

Oh, I've never heard of that symbol, I just googled it and saw that
Borland C++ defined it. If it's just to detect MinGW, it could use
`__MINGW32__`, or probably even `__GNUC__ && _WIN32`.

>> Geany is using these libraries on Windows now as they're what GTK+ recommends since they stopped providing binary bundles on their website, which is why I mentioned.
>
> Does Geany correctly paste rectangular selections copied from Visual Studio or SciTE as rectangular now?
>

AFAIK, in Geany rectangular pastes don't work correctly on any platform.
I don't have access to Windows at the moment to check if it behaves the
same as on Linux.

Regards,
Matthew Brush

[Neil Hodgson]

Matthew Brush:

> They're compiled with GCC rather than MSVC, but still for Windows using Win32 headers, C ABI and runtime. The usual _WIN32 should be defined (and G_OS_WIN32 from GLib).

…

> Geany is using these libraries on Windows now as they're what GTK+ recommends since they stopped providing binary bundles on their website, which is why I mentioned.

> Oh, I've never heard of that symbol, I just googled it and saw that Borland C++ defined it. If it's just to detect MinGW, it could use `__MINGW32__`, or probably even `__GNUC__ && _WIN32`.

Is Geany building inside the Msys2 shell or using the headers/libs from Msys2 but using a normal Windows cmd.exe shell?

Running “gcc -dM -E - </dev/null” to show preprocessor definitions from the gcc hosted by Msys2 shows “unix” and relatives defined but no “_WIN32” or “__MINGW32__". Putting #ifs inside the Scintilla source code shows the same thing.

Trying the same thing with MinGW-32 from cmd.exe has “_WIN32” and “__MINGW32__”.

Neil

[Matthew Brush]

I will try it next time I'm in Windows. I've built Geany before using
its Autotools build system in Msys2, but I can't say for sure how it
ended up setting the environment without trying it again.

Enrico does the official Windows release and could probably confirm, but
I think the binaries are built from within Msys2 shell.

P.S. I previously mentioned rectangular paste didn't work in Geany, I
must've been thinking of something else, it does work. I'll test it with
VS and SciTE next time I'm in Windows.

Regards,
Matthew Brush

[Neil Hodgson]

Colomban Wendling:

> 1. like yours, but just define ATK_CHECK_VERSION() if it's missing, so
> there is a single place where that compatibility is handled.

A simple addition of this to allow compiling easily on Windows committed here:
https://sourceforge.net/p/scintilla/code/ci/ec2ef0bc8382a925755b7c1da01a741cdab0ae16/

Since the inclusion of windows.h occurs after glib.h is included, it would probably be better to use G_OS_WIN32 to gate the inclusion instead of relying on checking the compiler.

Neil