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

Relate Categories to a series #414

Merged
merged 78 commits into from
Mar 14, 2022

Conversation

Arnei
Copy link
Member

@Arnei Arnei commented Sep 28, 2020

Intended to solve issue #260.

Categories are only related to the event they've been created on, but this event allows to create category related to the series of the event. A category belonging to a series will appear on all events belonging to that series, and changes made to the category will also be reflected on all events.

Technically, categories got two new fields: seriesExtId and seriesCategoryId. For an normal event category, these will be empty. If a category is related to a series, seriesExtId will contain the Opencast series UID of the event. seriesCategoryId will contain the ID of a master category. Changes made to a series category will also be made to the master category, and each updates it's categories based on the master categories. On a master category, seriesCategoryId is empty.

Since this PR requires new database columns, you'll likely have to update your current database: 260_upgrade_skript.zip

TODOs:

  • Bug: If a series category has a scale, the scale selection next to a label will appear bugged on other videos with the series category.
  • Bug: If you make changes to a series category on video A and then turn it back into an event category, the changes you made are not propagated to the respective category on video B. (Because the local copy of a series category only gets updated once on page load)
  • Optional: Streamline the code that loads the categories for the video.

Update 15.10:

  • Bug: The "sufficiently equal" check that should decide whether a non-series category is similar enough to a series category to be related to it, does not work.
  • Bug: Series categories on video "B" will sometimes need two page refreshes before the changes to the series categories made on video "A" are shown. This is the case if video "B" was not the video where the category was changed to be a series category.

Update 21.10:

  • Bug: When a series category is turned back into an event category it may loose it's labels. (When the reference to a series category is lost, the reference to the labels is lost as well. Unlike with categories, the labels are not copied to the video in that case)

Update 22.10:

  • Disabled scales for series categories. Meant as a temporary fix to avoid users running into weird error states, until the actual bug is resolved.

Update 26.11:

  • Properly disabled scales for series categories, as described by dagraf in his comment from the 25.11.2020.
  • Also changed icons and fixed translation problems as described in the same comment.

Update 16.12:

  • Turning a series category back into a video category should now delete copies on all other videos.
  • Custom delete message for series categories.

Update 13.01:

  • Timeline Annotations should now be correctly colored again.
  • List Annotations should now correctly switch color again.

Update 15.01:

  • Fixed a bug where it looked like duplicate categories were created due to the master series category being moved from video to video.

Update 28.01:

  • Fixed a bug where annotations created from series category copies would not show up in a CSV export. This fix requires a database update ! (ALTER TABLE xannotations_label ADD COLUMN series_label_id BIGINT;)

Arnei added 11 commits September 15, 2020 17:03
…e.g. Videos which change series.

- Also includes a hacky fix for query-strings
- Needs a throughouh clean-up
…ted-to-series

# Conflicts:
#	frontend/package-lock.json
If a category belongs to a series, all backend request for labels will now point to the labels of the master category
- Also tries to check for "sufficiently equal" categories and link them to series categories
- Tries to turn series categories into normal categories if their master is lost
@Arnei Arnei marked this pull request as ready for review October 13, 2020 10:12
Arnei added 2 commits October 14, 2020 17:48
- Fixed potential error with query parameters
- Revised getting categories in the frontend
- Fixed potential NullPointerException when getting series categoires in the backend
- Fixed category equals check in the backend
@Arnei Arnei changed the title WIP: Relate Categories to a series Relate Categories to a series Oct 19, 2020
Arnei added 3 commits October 21, 2020 10:41
- Should fix an issue where labels were lost after a series category was turned back into a normal category
This is a temporary workaround for the issue with scales not working correctly with series category.
Only meant as a quick fix since the actual fix would probably take way longer, and should be removed after.
* This effectively turns scales global, as every video will fetch all scales
* Undoes previous commit "Removed scales from series category" 91b40e7
@JulianKniephoff
Copy link
Member

I just deployed this on SWITCH staging and it revealed some other questionable things in the tool: It initially requests all the scales in the system (which is what we want) and then goes through each scale individually and downloads the scale values for it. This happens in one synchronous request per scale. Since there are apparently somewhere between 300 and 400 scales in the entirety of the SWITCH system, this takes a while.

The tool seems to hang, but it will load eventually, so you can still test, @dagraf. But of course this is unacceptable and we need to come up with a solution. 🤔

@dagraf
Copy link
Collaborator

dagraf commented Nov 24, 2020

I'm now trying to open a video in the annotation tool. The waiting time is indeed unacceptable. What would you propose how to solve it?

@dagraf
Copy link
Collaborator

dagraf commented Nov 24, 2020

I am still waiting the video to be loaded...

What about making scales not global but "regional" = always belonging to a series?

@JulianKniephoff
Copy link
Member

I have a few ideas:

  • I haven't looked into it, but my theory is that a lot of these scales are in some way duplicates or at least similar enough so that one could "fuse" them together. We would need to do some digging into the data here. If there are duplicates or near-duplicates, we can just delete all the "copies" except one, and point all of the annotations using them to the "blessed survivor". This would hopefully reduce the number of scales the tool needs to fetch in the beginning.
  • Instead of sending one request per scale (to get the scale values), we could just deliver the scale values together with the scales, somehow. This would cut down on the overhead of doing it one scale at a time. There are multiple ways to do this:
    • Just change the API so that /scales already contains the scale values for each scale. This would require some changes in the frontend as well, to make it compatible again.
    • Play around with HTTP2 Server Push. This would be the more unobtrusive solution if we can get it to work, but I don't know whether all the components in our HTTP chain support this.
  • Make the AJAX requests used to fetch the scale values async, so that we don't have to wait for each one in turn. We would still have to wait for all of them to be loaded, except we ...
  • Restructure the frontend so that it doesn't need all the scales/scalevalues at the start. This one is a bit fuzzy; there are a lot of ways to approach this and it's not clear how to go about this optimally. It is also the approach that is most prone to error. In a way, the current approach of the tool where it basically downloads all the data once in the beginning is conceptually very simple. The tool unfortunately doesn't always fully utilize this simplicity, so instead of making it more complex, we might actually "formalize" this approach much more, by just grabbing everything (not just the scales) in one request in the beginning, and never have to worry about the back end after the loading screen is gone, except for saving stuff, of course. But I digress ...

The point about the async AJAX requests is one we have to attack at some point, anyway; not only for scales, but for everything. The tool unfortunately makes a lot of synchronous requests, which make the code somewhat simpler, but also rather slow, to the extent where the tool can become unresponsive if a request takes long due to a sudden case of "bad internet", because the request even blocks the rendering thread. Also Chrome warns about deprecating sync AJAX requests for years already; they might just stop working at some point.

@JulianKniephoff
Copy link
Member

JulianKniephoff commented Nov 24, 2020

What about making scales not global but "regional" = always belonging to a series?

There are a few problems with this:

  1. There can be videos without series, so we need to at least manage per-event scales and per-series scales.
  2. Like with the categories and labels, we have to deal with the fact that the series of a video might change under our noses, and it might even change between "some series" and "no series", so we need to be able to "move" scales between series and between a series and a video.

These two points already make the whole thing at least as complicated as the category/label stuff, but it gets better:

  1. Unlike a label, which only ever belongs to exactly one category, a scale can exist divorced from any category, or even be assigned to multiple categories. So while for a label it's always clear whether it belongs to an event or a series (it just inherits this attribute from its parent category) a scale can be assigned to a series category, an event category, or both, all while itself being either a series scale or an event scale. This opens up a whole other dimension of design space we need to think through.

It is probably possible to think through all of this and then carefully implement it using a similar "trick" we use for the categories/labels, i.e. maintaining a copy of every series category per video. We would need to extend it, though, to apply it to scales as well. And that "trick" is already a pretty bad hack, to be honest. I would be very reluctant to add an even bigger version of that on top. In fact, we should probably think about another way to solve the per series categories thing in the long run, but there are quite a few refactorings I would want to do before attempting that.

And lastly, due to all the complexities/problems mentioned above, this is not something we can do in the remaining ~10 or probably even in 20 hours. ☹️

@dagraf
Copy link
Collaborator

dagraf commented Nov 25, 2020

Umgang mit Skalen:

  1. Icon Kategoriezugehörigkeit (siehe Anhang, grau unterlegt = gewünschtes Icon-Paar; SVG-Dateien werden per Mail geschickt)
    Auswahl

  2. Sprachmitteilung Tool-Tip Kategoriezugehörigkeit:

  • Serie: «Kategorie gehört zur Serie & erscheint bei allen Videos» / «Category belongs to the series & is shown in all videos»

  • Event: «Kategorie gehört zu diesem Video» / «Category belongs to this video»

  1. Lösung für «Skalen sind nur für Video-Kategorien möglich»:
  • Kategorie gehört nur zu einem Video: Alles wie gehabt.
  • Kategorie gehört zu einer Serie: (i) «Stift»-Icon wird angeboten, bei Klick darauf öffnet sich ein Modal mit der Sprachmitteilung: «Diese Kategorie gehört zu einer Serie. Daher kann ihr keine Skala zugeordnet werden.» / «This category belongs to a series. Therefore, assigning a scale it is not possible.» (ii)Button unten rechts: «Schliessen» / «Close»
  • Wenn eine Kategorie eine Skala besitzt: (i) Icon zur Wechsel der Kategoriezugehörigkeit ist ausgegraut. (ii)Tool-Tip bei Hoverover: «Kategorie verfügt über eine Skala. Wechsel der Zugehörigkeit der Kategorie nicht möglich.» / «Category has a scale. Changing category affiliation not possible.»

Arnei added 6 commits November 26, 2020 10:17
- Undid the previous commit for global scales
- Will now display modals with warning messages to prevent the user from creating a series category with a scale
...by having them run on the labels of the master series category.
But also still creating local copies of labels in case the affinity should change back to a normal video category.
@JulianKniephoff JulianKniephoff merged commit ce7c909 into opencast:master Mar 14, 2022
@unterrichtsvideos unterrichtsvideos mentioned this pull request Jul 26, 2023
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.

None yet

3 participants