New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes and improvements for the Zoom accessibility feature #1242

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@dusek
Contributor

dusek commented Jul 12, 2014

Please see commit messages for details.

I release these patches into the public domain.

dusek added some commits Jul 12, 2014

layout: Fix bol_as_eol for case where range length is 0
bol_as_eol was meant for non-empty ranges, but the only place where
empty ranges were attempted with bol_as_eol was for Zoom tracking,
and it seems like I did not re-test Zoom after making the bol_as_eol
commit.

Fixes textmate/bugs#4
Fix textmate/bugs#5
When setSelectionString is called during editing of text (like inserting
or deleting a character), it catches layout in a semi-consistent state
where horizontal position of individual characters cannot be determined.
Therefore during such events, we cannot pass the correct screen position
to the call to UAZoomChangeFocus.

So let's do the UAZoomChangeFocus call after this iteration of run loop.
This ensures everything in layout that had to update has updated when
calling UAZoomChangeFocus, thus enabling us to provide correct horizontal
screen position.
Optimize Zoom support a little bit
This makes Zoom support avoid doing all 2 conversions between UTF-8 to UTF-16,
and replaces call to rect_for_range with call to rect_at_index, which is
more efficient.

Note that this is a typical micro-optimization not resulting from any
actual performance measurement, but just from a desire to have a lean
code not doing redundant work.
@sorbits

This comment has been minimized.

I am curious as to why you prefer dispatch_async with the main queue over performSelector:withObject:afterDelay:.

This comment has been minimized.

Owner

dusek replied Jul 15, 2014

I did not spend much time deciding between the two, I googled a little bit and discovered that performSelector:withObject:afterDelay: with delay of 0 might actually still work using a timer (instead of queueing up the message for execution during next iteration of run loop), so dispatch_async felt a little more efficient and what I wanted.

But I am no expert in this topic, so if you feel performSelector:withObject:afterDelay: is the appropriate way, I will rework it to use that method.

This comment has been minimized.

Owner

dusek replied Jul 15, 2014

Actually just read the docs and it is not 100% clear whether a timer is used with delay of 0, but says the selector is queued to be executed ASAP, so not sure what to use.

This comment has been minimized.

sorbits replied Jul 15, 2014

This comment has been minimized.

Owner

dusek replied Jul 15, 2014

I will do it this evening, after testing the change I will make a new commit just with the new change (instead of git push --force with the change squashed into this commit, in order not to mess up this conversation history on GitHub issues). I will look out for the new line ;-)

@sorbits

This comment has been minimized.

sorbits commented on 2d6c0f3 Jul 15, 2014

I wouldn’t call this a “micro-optimization”, as O(n) code has been replaced with API that is expected to be O(lg N) or better.

FYI the old code did cause a noticeable slowdown (when zoom was enabled) for large files (multiple hundreds of kilobytes), so this improvement is much appreciated :)

This comment has been minimized.

Owner

dusek replied Jul 15, 2014

I am glad to hear that (I did not test with big files). :-)

dusek added some commits Jul 15, 2014

fixup! Fix textmate/bugs#5
Use `performSelector:withObject:afterDelay:` instead of dispatch_async
for `UAZoomChangeFocus` (Zoom focus change notifications).
fixup! Fix textmate/bugs#5
Remove extra newline after a method
@dusek

This comment has been minimized.

Contributor

dusek commented Jul 15, 2014

I added 2 new commits, one for the performSelector:withObject:afterDelay:, second for the newline.
The are marked as fixup! for immediate git rebase --interactive --autosquash master..

@sorbits

This comment has been minimized.

Member

sorbits commented Jul 19, 2014

Thanks, merged as 754f1a1...7e01bab.

I added a (dummy) argument to updateZoom: so that the prototype matches what is expected when using performSelector:withObject:afterDelay:.

@sorbits sorbits closed this Jul 19, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment