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

(2/3) Bugfixes #604

Conversation

unterrichtsvideos
Copy link

@unterrichtsvideos unterrichtsvideos commented Apr 3, 2023

This branch builds upon the merged version from PR #603

The Bern (B, current master) and Münster (MS, questionnaire branch) versions had serveral bugs which were fixed after the merge, e.g.

  • The export function was broken due to introduction of MCA (MS)

  • A scale could not be edited (B)
    image

  • A shared category ("with all users") could not be edited from the same user (B)
    image

  • An assigned scale could not be used (B)
    image

  • The new edit menu was out of bounds / cut off (B)
    image

see commits for further bug fixes

mlunzena and others added 30 commits May 26, 2020 08:00
`label.category` is only set for preexisting categories,
not for ones created during the current browser session.
With this patch, the frontend also knows about deleted labels,
categories, scales and scale values. This makes the behavior of
annotations carrying any of that deleted structure much more
consistent.
Previously it already showed the editing interface for the scale
of the category you got to the dialog with. It wasn't fully in edit
mode yet, though, so edits could get lost.
Arnei and others added 4 commits July 24, 2023 11:40
Make every caller pass their preferred type instead of doing a cast.
Co-authored-by: Julian Kniephoff <[email protected]>
Co-authored-by: Julian Kniephoff <[email protected]>
Fix db compatibility issues with Opencast 13
@Arnei
Copy link
Member

Arnei commented Jul 26, 2023

Played around with this a bit, here are few things I've noticed:

  • When creating an annotation from a questionnaire and setting the start or end time, a user can enter times that don't conform to the format 0:00:00. The user is notified of this with a format error. But even then, you can still hit save and the Annotation Tool will make a best effort guess at what you were going for.
    • E.g. entering "60" is interpreted as "0:00:06". Not what I would have expected
  • If a questionnaire has a public category and a private category as a label, it is treated as private. Is this expected behaviour?
  • When switching to the questionnaire layout, the annotation list is not shown, even though the view dropdown claims that it is visible somewhere
    • Bildschirmfoto vom 2023-07-26 10-34-33
  • Currently the only way to get the questionnaire view to show up is by selecting the questionnaire layout. It would be nice if I could add the view to my current layout just like e.g. "Loop controller".
  • Unrelated to the questionnaire, categories (and their labels) that belong to a series may sometimes get duplicated. Not quite sure when this is happening, seems to be related to adding/removing videos from series.
  • Bildschirmfoto vom 2023-07-26 10-28-39

* Minimally improve the formatting
* Remove useless settings
This necessitates quoting some column names.

Also restrict this dependency to the `test` scope.
@unterrichtsvideos
Copy link
Author

unterrichtsvideos commented Jul 26, 2023

Thanks Arnei for having a look into it.

When creating an annotation from a questionnaire and setting the start or end time, a user can enter times that don't conform to the format 0:00:00. The user is notified of this with a format error. But even then, you can still hit save and the Annotation Tool will make a best effort guess at what you were going for.

  • E.g. entering "60" is interpreted as "0:00:06". Not what I would have expected

Bug or feature? 🧐

Notes for readers:

  • In this context "Save" means "setting the annotation end" (blue arrow, see image below). Trying to save the whole template-based annotation with an invalid value as annotation end will instead result in a format error note asking to correct the value.
  • Suggest: The behavior of the annotation save button should be adopted for the "annotation end" - save button; this could be done after a merge as the other behavior could be tolerated for now?
"Annotation end" save button Annotation save button triggers format error note

  • If a questionnaire has a public category and a private category as a label, it is treated as private. Is this expected behaviour?

This seems plausible for me to some extend. Regular MCA (without annotation template) should behave the same.


  • When switching to the questionnaire layout, the annotation list is not shown, even though the view dropdown claims that it is visible somewhere

Which branch did you review? The annotation list should be shown in 3/3 branch; commit d089bea should have fix that (show the annotation list in annotation template view).


  • Currently the only way to get the questionnaire view to show up is by selecting the questionnaire layout. It would be nice if I could add the view to my current layout just like e.g. "Loop controller".

This should be possible in the branch mentioned above:

  • View -> "Edit Annotation templates" should create a new Tab in the layout

There are also some "hidden" ways to show the areas whenever it is useful, e.g.

  • Timeline -> Double click/tap an annotation in timeline created by a template opens template editor
  • List of annotations -> Double click/tap an annotation created by a template opens template editor
View options Tabs
image image

  • Unrelated to the questionnaire, categories (and their labels) that belong to a series may sometimes get duplicated. Not quite sure when this is happening, seems to be related to adding/removing videos from series.´

This should be a known bug originating from the current master version which introduced sharing categories with series leading to this duplication. As this bug should already exist in the master version and was not introduced by the merge, this should not be handled as a blocker for the merge but should be looked into later on.

Some details on that matter from internal discussions which might be outdated:

  • There was another duplication bug which should be fixed by this commit; also this documentation
  • There was a related bug in this duplication context which lead to oat stopped working upon load which was worked around by this commit
  • We suspected the issue to be in ExtendedAnnotationServiceJpaImpl.java : getLabels
  • Labels get duplicated, original labeles get marked as "deleted" in DB

Reproduction hints:

  • Video1, Series1: Create category, share with Series1
  • Video2, Series1, Create annotation with shared category
  • Reload Video2

The error (duplicates) occurs e.g. (not only) as soon as another video of the series gets the series category (as annotation).

Possible Request triggering the incident:

  • /extended-annotations/videos/.../categories/.../labels?series-extid=...

Note:
I'd suggest to review the version resulting from the merge of all three branches, the PRs are directed to opencast:merge instead of master/main branch

@Arnei
Copy link
Member

Arnei commented Jul 27, 2023

Bug or feature? monocle_face

Notes for readers:

In this context "Save" means "setting the annotation end" (blue arrow, see image below). Trying to save the whole template-based annotation with an invalid value as annotation end will instead result in a format error note asking to correct the value.
Suggest: The behavior of the annotation save button should be adopted for the "annotation end" - save button; this could be done after a merge as the other behavior could be tolerated for now?

Oh, so the save button for the whole template does do time stamp validation, did not know that. Arguably that is not very helpful, as it asks you to ignore the prominent "annotation end" save button. Which you cannot do if you also want to edit "annotation start".

I'd be in favor of your suggestion, or just doing away with the "annotation end" save and cancel buttons

Which branch did you review? The annotation list should be shown in 3/3 branch; commit d089bea should have fix that (show the annotation list in annotation template view).

Reviewed this branch (2/3), thanks for telling me about the fix in 3/3.

This should be a known bug originating from the current master version which introduced sharing categories with series leading to this duplication. As this bug should already exist in the master version and was not introduced by the merge, this should not be handled as a blocker for the merge but should be looked into later on.

Agreed, not a blocker then. Should have checked if that issues already existed, my bad.

@unterrichtsvideos
Copy link
Author

I'd be in favor of your suggestion, or just doing away with the "annotation end" save and cancel buttons

+1
I think it's not necessary to save the annotation start or end separatly. The implementation is based on the original MCA concepts and code from the old questionnaire branch.

Agreed, not a blocker then. Should have checked if that issues already existed, my bad.

No worries, I'd suggest to check the described behavior in the master version again to make sure it's the same bug.

Arnei and others added 16 commits July 28, 2023 09:39
* Minimally improve the formatting
* Remove useless settings
That `when` was in the wrong place.
This makes the tests work with newer Java versions (tested with 20)
and also fixes a warning about invalid POMs during the Maven build.
When building with Maven's `-T` option the build was warning
about two plugins that might not be thread safe.

This patch updates one and removes the other.

Regarding static weaving: It might be interesting to look at
at some point, but it's "only" for performance and Opencast removed
this as well: opencast/opencast#3031.
Also some minor nitpicks and fixing of lints.
@JulianKniephoff JulianKniephoff deleted the branch opencast:merge December 13, 2023 17:00
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.

6 participants