Skip to content
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

override peek implementation for libreoffice #1532

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chdoc
Copy link

@chdoc chdoc commented Aug 8, 2024

This only matches for libreoffice on Linux. I don't have access to Windows or Mac OS, so I don't know what libreoffice calls itself on those operating systems.

mod.apps.libreoffice = """
os: linux
and app.exe: soffice.bin
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add the windows matcher

os: windows
and app.exe: soffice.bin

return (before, after)


def repeat_action(action: Callable, count: int):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this function and just inline it. It's actually fewer lines.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think inlining this increases clarity. In fact, I copied the function from core/edit/edit_navigation_steps.py. So I would prefer moving this to the right place instead, whatever the right place is.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Andreas, I found this harder to parse. For a two-line function I agree we should just inline it - otherwise it leads the reader to try to understand why the function was necessary (performance? etc).

While edit_navigation_steps.py does have many functions like this, it has so many that the pattern becomes more understandable.

apps/libreoffice/libreoffice.py Show resolved Hide resolved
@AndreasArvidsson AndreasArvidsson removed their assignment Aug 11, 2024
@chdoc
Copy link
Author

chdoc commented Aug 20, 2024

Apologies for being silent on this for so long. I realized that, while this works really well for a normal text, it still fails miserably when editing text in tables: then running out of words actions.edit.extend_word_left transitions to selecting entire table cells, and then all bets are of. Not sure what to do about this.

@AndreasArvidsson
Copy link
Collaborator

Feedback from the community backlog session. If we can't actually get this to work reliably in both text and tables we are kind of leaning towards actually just disabling context sensitive dictation by default in libre office with a comment about the failure mode for tables. Then we can actually merge this change and if people want to use it it's up to them.

return (before, after)


def repeat_action(action: Callable, count: int):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Andreas, I found this harder to parse. For a two-line function I agree we should just inline it - otherwise it leads the reader to try to understand why the function was necessary (performance? etc).

While edit_navigation_steps.py does have many functions like this, it has so many that the pattern becomes more understandable.

@AndreasArvidsson
Copy link
Collaborator

Looks like clip.mime().html has s etc for a table selection. We could potentially use this to determine if we are in a table and in that case don't do this cursor dance.

@chdoc
Copy link
Author

chdoc commented Sep 2, 2024

Looks like clip.mime().html has s etc for a table selection. We could potentially use this to determine if we are in a table and in that case don't do this cursor dance.

I don't understand how this is supposed to work. After all, by the time we can use clip to detect whether we have selected a table, we're already in the middle of the "cursor dance".

That being said, I disagree that "just disabling smart insertion by default" is a viable option. I find it absolutely indispensable when editing normal text, even though it is utterly useless (or a major hindrance) in certain contexts. Maybe this shouldn't be a fixed setting for a given application, instead there should be a command to quickly toggle that. How would one go about implementing this?

@AndreasArvidsson
Copy link
Collaborator

AndreasArvidsson commented Sep 2, 2024

It might be enough to select a single character and then get the html text. This would be a pre stage before doing any further "cursor dance". We are not sure if this is a viable solution, but might be worth looking into.

All Talon settings are already context based. This means you can enable or disable context sensitive dictation in any application or other matchable context. What we suggested was that it by default should be specifically disabled in libre office with a comment describing why. People can still enable it if they want to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants