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

Remove all segments button #1097

Merged
merged 8 commits into from
Sep 21, 2023
Merged

Conversation

dennis531
Copy link
Collaborator

@dennis531 dennis531 commented Jul 4, 2023

Contains the following changes:

  • Adds a button to delete all segments
  • Adds a button for each segment to delete corresponding segment

Ref #1083

@dennis531 dennis531 added the type:feature A new feature or feature request label Jul 4, 2023
@github-actions
Copy link

github-actions bot commented Jul 4, 2023

This pull request is deployed at test.editor.opencast.org/1097/2023-07-04_07-56-11/ .
It might take a few minutes for it to become available.

@dennis531 dennis531 requested a review from Arnei July 4, 2023 08:42
@Arnei
Copy link
Member

Arnei commented Jul 4, 2023

Short comment:

A "Delete All" Button seems completely superfluous to me. What's the use case?

Also don't like the buttons in the segments. Small, so hard to hit with a mouse. Overlap issues for a series of smaller cuts. Hardly read as buttons. I get that first clicking into a segment before you can click on the delete button is not the best UX, but that's exactly what the keyboard shortcuts are for.

@lkiesow
Copy link
Member

lkiesow commented Jul 4, 2023

I think you misunderstood #1083. I think what @kweingaertne wants is not to mark the content for removal, but to be able to quickly remove all segments itself. Right now, you would do that by starting somewhere and hitting the “Merge Left/Right” buttons over and over again.

Marking all segments for removal doesn't really make sense because you would end up with a 0 seconds long video file which should then fail to process as Opencast would tell you that this doesn't really make sense :D

@lkiesow
Copy link
Member

lkiesow commented Jul 4, 2023

I'm also not the biggest fan of the icon on the timeline. The old editor had several of them and I would like to not see them creep back one after another. Maybe instead as a “pro” feature to easily access the functionality implement #943 instead?

What do you think, @kweingaertne?

@lkiesow
Copy link
Member

lkiesow commented Jul 4, 2023

Finally (for the next time), having one feature/change pre pull request makes these changes easier to discuss, review and merge.

@github-actions
Copy link

github-actions bot commented Sep 4, 2023

This pull request is deployed at test.editor.opencast.org/1097/2023-09-04_10-46-22/ .
It might take a few minutes for it to become available.

@dennis531
Copy link
Collaborator Author

dennis531 commented Sep 4, 2023

Now the button Merge All allows to merge all segments into a single segment.
Additionally, the delete button on each segment is removed.

@dennis531 dennis531 marked this pull request as ready for review September 4, 2023 12:04
Copy link
Member

@Arnei Arnei left a comment

Choose a reason for hiding this comment

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

Looks good to me, code-wise and all. Just found one small behavioural change.

Usually when merging segments, the resulting segment inherits the "deleted state" of the previously selected segment. So if I select a "blue" segment and merge it with "red", the resulting segment is "blue". This is not true for the specific case in which I select a "red" segment" and merge it left with a "blue" segment. The resulting segment is "blue" instead of "red". Could you fix that?

@dennis531
Copy link
Collaborator Author

Usually when merging segments, the resulting segment inherits the "deleted state" of the previously selected segment. So if I select a "blue" segment and merge it with "red", the resulting segment is "blue". This is not true for the specific case in which I select a "red" segment" and merge it left with a "blue" segment. The resulting segment is "blue" instead of "red". Could you fix that?

Thanks for noticing. It should be fixed now.

@dennis531 dennis531 requested a review from Arnei September 4, 2023 13:49
@github-actions
Copy link

github-actions bot commented Sep 4, 2023

This pull request is deployed at test.editor.opencast.org/1097/2023-09-04_13-53-50/ .
It might take a few minutes for it to become available.

@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Sep 13, 2023
@github-actions
Copy link

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@github-actions github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Sep 21, 2023
@github-actions
Copy link

This pull request is deployed at test.editor.opencast.org/1097/2023-09-21_09-50-49/ .
It might take a few minutes for it to become available.

@dennis531
Copy link
Collaborator Author

Usually when merging segments, the resulting segment inherits the "deleted state" of the previously selected segment. So if I select a "blue" segment and merge it with "red", the resulting segment is "blue". This is not true for the specific case in which I select a "red" segment" and merge it left with a "blue" segment. The resulting segment is "blue" instead of "red". Could you fix that?

Thanks for noticing. It should be fixed now.

During solving the merge conflicts, I have noticed that this problem was still present. Hopefully, this is solved now.

@Arnei Arnei merged commit 9d1662e into opencast:main Sep 21, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature A new feature or feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants