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

Fix/window event race condition #842

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

Conversation

Animesh404
Copy link
Contributor

@Animesh404 Animesh404 commented Jul 8, 2024

What kind of change does this PR introduce?

a bugfix for #816

Summary

  • removed data from WindowEvent model and created a new model A11yEvent which points to the relevant WindowEvent
  • updated crud.py by adding insert_a11y_event
  • made changes in record.py where we use queue to map which WindowEvent is waiting for its corresponding A11yEvent

Checklist

  • My code follows the style guidelines of OpenAdapt
  • I have performed a self-review of my code
  • If applicable, I have added tests to prove my fix is functional/effective
  • I have linted my code locally prior to submission
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. README.md, requirements.txt)
  • New and existing unit tests pass locally with my changes

Copy link
Member

@abrichr abrichr left a comment

Choose a reason for hiding this comment

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

Thanks @Animesh404 ! I left a few comments.

Can you please also test out https://github.com/Kalmat/PyWinCtl ? I believe it should also support MacOS.

openadapt/window/_windows.py Outdated Show resolved Hide resolved
openadapt/window/_windows.py Outdated Show resolved Hide resolved
openadapt/window/_windows.py Outdated Show resolved Hide resolved
openadapt/window/_windows.py Outdated Show resolved Hide resolved
openadapt/window/_windows.py Outdated Show resolved Hide resolved
openadapt/window/__init__.py Outdated Show resolved Hide resolved
openadapt/window/_windows.py Outdated Show resolved Hide resolved
@Animesh404 Animesh404 marked this pull request as draft July 16, 2024 21:29
@Animesh404 Animesh404 marked this pull request as draft July 16, 2024 21:29
openadapt/models.py Outdated Show resolved Hide resolved
openadapt/record.py Outdated Show resolved Hide resolved
openadapt/record.py Outdated Show resolved Hide resolved
@abrichr
Copy link
Member

abrichr commented Jul 18, 2024

@Animesh404 please consolidate the alembic scripts into a single migration. You can do this by downgrading the two that you have currently applied:

alembic downgrade -2

Then generate a new revision, add it, and remove the existing ones.

openadapt/models.py Outdated Show resolved Hide resolved
openadapt/models.py Outdated Show resolved Hide resolved
openadapt/models.py Outdated Show resolved Hide resolved
openadapt/record.py Outdated Show resolved Hide resolved
openadapt/record.py Outdated Show resolved Hide resolved
openadapt/record.py Outdated Show resolved Hide resolved
openadapt/models.py Outdated Show resolved Hide resolved
openadapt/models.py Outdated Show resolved Hide resolved
@Animesh404 Animesh404 marked this pull request as draft July 25, 2024 11:52
@Animesh404 Animesh404 marked this pull request as ready for review July 25, 2024 17:11
@abrichr
Copy link
Member

abrichr commented Jul 25, 2024

Thanks @Animesh404 ! Can you please show screenshots of a11y events in visualize.py and the dashboard?

@Animesh404
Copy link
Contributor Author

Thanks @Animesh404 ! Can you please show screenshots of a11y events in visualize.py and the dashboard?

Sure @abrichr

  • Dashboard
    image
  • Visualize
    image

@abrichr
Copy link
Member

abrichr commented Jul 25, 2024

@Animesh404 please correct me if I'm wrong but I don't see any accessibility data in the dashboard screenshot

@Animesh404 Animesh404 marked this pull request as draft July 26, 2024 08:58
@Animesh404
Copy link
Contributor Author

@Animesh404 please correct me if I'm wrong but I don't see any accessibility data in the dashboard screenshot

image
added here 3b0ada0

@Animesh404 Animesh404 marked this pull request as ready for review July 29, 2024 16:55
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.

2 participants