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

Add Playback times #196

Open
wants to merge 22 commits into
base: dev
Choose a base branch
from
Open

Conversation

azfoo
Copy link
Collaborator

@azfoo azfoo commented Oct 10, 2024

#180, closes #178.
WIP:
fix CLI dialog

@azfoo azfoo self-assigned this Oct 10, 2024
@azfoo
Copy link
Collaborator Author

azfoo commented Oct 15, 2024

@FelipeDefensor, need a little help on this one: tests.ui.cli.test_load_media
in trying to avoid the dialog popping up on the CLI, I've called Get.WINDOW_STATE and checked for a NoReplyToRequest (because it is only served in QtUI and not the CLI), which works with the actual CLI, but fails on the test. Somewhere in the setup of the test, QtUI is initialised and is replying. Creating a new Get.UI_TYPE also fails because both UIs give conflicting responses. Any ideas on how to solve this?

@FelipeDefensor
Copy link
Collaborator

@FelipeDefensor, need a little help on this one: tests.ui.cli.test_load_media in trying to avoid the dialog popping up on the CLI, I've called Get.WINDOW_STATE and checked for a NoReplyToRequest (because it is only served in QtUI and not the CLI), which works with the actual CLI, but fails on the test. Somewhere in the setup of the test, QtUI is initialised and is replying. Creating a new Get.UI_TYPE also fails because both UIs give conflicting responses. Any ideas on how to solve this?

Will look into it.

@FelipeDefensor
Copy link
Collaborator

FelipeDefensor commented Oct 17, 2024

Ok, so this is entirely on me. Contrary to what you would expect, the marker_tl fixture requests the marker_tlui fixture that request the qtui fixture. This is first-class spaghetti that I need to unravel.

There were improvements to the tangled fixtures in v0.4.3, but apparently they were'nt enough. I will see if I can provide a permanent solution in the next few days. You can wait for that, or skip those tests with @pytest.mark.skip if you want to move on with this PR. Let me know.

@azfoo
Copy link
Collaborator Author

azfoo commented Oct 17, 2024

Marked skip for now.
Do review if you have time - don't think there's much change in the code apart from the merge/rebase.

@FelipeDefensor
Copy link
Collaborator

There were improvements to the tangled fixtures in v0.4.3, but apparently they were'nt enough. I will see if I can provide a permanent solution in the next few days. You can wait for that, or skip those tests with @pytest.mark.skip if you want to move on with this PR. Let me know.

Easier than I thought, so I already pushed a commit to dev dealing with this.

Will review this PR later today.

@FelipeDefensor
Copy link
Collaborator

Anne, sorry for the delay on the review. I was part of a congress today that took longer than I expected. Will review tomorrow, guaranteed.

accidentally discarded - 97bf4aecd92f180f0e892e70c5e1c403f6aca096
display a subsection of input media. set through metadata window.
ScaleOrCrop dialog replaces the old double dialog
Copy link
Collaborator

@FelipeDefensor FelipeDefensor left a comment

Choose a reason for hiding this comment

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

The code is pretty clean and everything seems to be working. I tried to be more thorough than normal in the review as experience (or trauma) tells me that media loading bugs tend to be hard to spot.

I will try to write a billion tests for this. In the meantime, there are some minor issues to be adressed.

Good job!

azfoo and others added 5 commits October 22, 2024 14:43
remove unneccesary data & monkeypatching
elementry test on PointLikeTimelineComponent and SegmentLikeTimelineComponent
`pre_start` and `post_end` need to stored before changing `start` and `end`
Something goes wrong when scaling multiple a segmentlikes.
In that way components will have the same duration they were created in to be restored to.
Copy link
Collaborator

@FelipeDefensor FelipeDefensor left a comment

Choose a reason for hiding this comment

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

The changes are perfect :).

Few new bugs I found:

  • Pre-start and post-end were not being scaled correctly. I fixed that with a commit.
  • Something goes wrong shen scaling multiple segmentlikes. I added a test for that behavior.
  • Undoing a media load with scale or crop triggers the dialog again. We could either reset the undo history when loading a new media or somehow store whether the user chose the scale or crop options. I leave that up to you.

We are almost there!

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