[video] Adjust the validations and messages in add version/extra #24477

CrystalP · 2024-01-10T11:25:30Z

Description

Reviewed, refactored, added messages. clarified messages related to the addition of a version or extra from library or files.
Both Manage versions > Add and the popup for similar movie found on scan are taken care of.
Manage versions > Add can add from library, relatively straightforward, but can also add any video file, which can be in many situations and require lots of validations and cases (videos is unknown, video is in library and is an extra, video is in library and is a version - could be default version or secondary version of a movie with only one version or multiple).

The design would need to change with the addition of a new dialog to clarify what's happening in some of the situations and avoid some overly long and not so clear messages. Only so much can be done with the available dialogs and space available in a popup yesno / OK dialog.

Messages changes:

reviewed and tried to simplify some
duplicated and specialized some messages because the same actions can be done to add an extra or a version and the message needs to reflect it.

Addressed functionality gaps:

adding movies of the library as new version or extra was silently losing the extras and non-default versions of the movie being added
through Browse files, a version can be turned into an extra and vice-versa. It has its uses but warrants a warning.
selecting the default version of a movie already in library in browse files removed the library entry and made the default version a version of the new movie, but any extras were silently discarded.

Future todo: the code for versions and extras was made very similar on purpose, it will be possible to group them in one function.

Motivation and context

Noticed some misleading messages and that code had to be reviewed.

How has this been tested?

all combinations of adding version or extra from:

library
files: unscanned/unknown video
files: video that's already an extra of a movie
files: video that's already an version of a movie
files: video that's a single version movie
files: video that's a the default version of a movie with multiple versions

What is the effect on users?

Better messages, error checking and handling of the addition.

Screenshots (if appropriate):

upcoming

Types of change

Bug fix (non-breaking change which fixes an issue)
Clean up (non-breaking change which removes non-working, unmaintained functionality)
Improvement (non-breaking change which improves existing functionality)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that will cause existing functionality to change)
Cosmetic change (non-breaking change that doesn't touch code)
Student submission (PR was done for educational purposes and will be treated as such)
None of the above (please explain below)

Checklist:

My code follows the Code Guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
I have read the Contributing document
I have added tests to cover my change
All new and existing tests passed

ksooo

Nothing serious.
I spotted some code fragments that look very similar. Maybe here is some potential to factor out something to get rid of duplicate code?

addons/resource.language.en_gb/resources/strings.po

xbmc/video/VideoDatabase.cpp

ksooo · 2024-01-10T21:43:46Z

xbmc/video/VideoDatabase.cpp

-int CVideoDatabase::GetVideoVersionInfo(const std::string& fileNameAndPath,
-                                        int& idFile,
-                                        std::string& typeVideoVersion,
-                                        int& idMedia,
-                                        MediaType& mediaType,
-                                        VideoAssetType& videoAssetType)
+CVideoAssetInfo CVideoDatabase::GetVideoVersionInfo(const std::string& filenameAndPath)


Thanks you so much for this interface change.

ksooo · 2024-01-10T21:47:20Z

xbmc/video/VideoManagerTypes.h

+public:
+  int m_idFile{-1};
+  int m_assetTypeId{-1};
+  std::string m_assetTypeName;
+  int m_idMedia{-1};
+  MediaType m_mediaType{MediaTypeNone};
+  VideoAssetType m_assetType{VideoAssetType::UNKNOWN};


I consider a class with all members being public not a good design. Either you make the new type a struct(and name it SVideoAssetInfo and remove m_ prefix from the members) or you provide proper getters and setters and make the members private.

Yes good idea. the code guidelines don't require the S prefix for the struct name, but still want the m_ prefix for the members...

Okay, we ofc should follow the code guidelines.

xbmc/video/dialogs/GUIDialogVideoManagerExtras.cpp

xbmc/video/dialogs/GUIDialogVideoManagerVersions.cpp

CrystalP · 2024-01-10T23:26:01Z

I spotted some code fragments that look very similar. Maybe here is some potential to factor out something to get rid of duplicate code?

Yes that is intentional to facilitate a refactoring. Only few details vary. I may even add a commit to this PR for it.

CrystalP · 2024-01-11T09:12:57Z

Suggestions applied, rebased + final commit to avoid losing extras when moving a movie into another one through Add > Browse files. Left the code merge between versions and extra dialogs for future PR, I need to recharge a bit, this was pretty complicated.
However for the user it is relatively simple (except a couple messages for weird situations) and the former silent loss of some video assets is avoided.
The design must change in v22.

ksooo · 2024-01-11T09:26:48Z

xbmc/video/dialogs/GUIDialogVideoManagerExtras.cpp

@@ -55,7 +55,6 @@ bool CGUIDialogVideoManagerExtras::OnMessage(CGUIMessage& message)
      {
        AddVideoExtra();
      }
-      break;


I would keep this break;, although not having it does not cause any problems atm. But if another case gets added in the future, one can easily forget to reintroduce the break here.

ok I didn't mean to remove it.

xbmc/video/dialogs/GUIDialogVideoManagerExtras.cpp

xbmc/video/dialogs/GUIDialogVideoManagerVersions.cpp

CrystalP · 2024-01-11T14:32:35Z

comments + formatting applied.

…extra or vice-versa

…nfirmation)

CrystalP · 2024-01-11T18:44:43Z

Added todo for future merge of add version and add extras logic

…iles

Stopped being used after PR xbmc#24477

CrystalP added Type: Fix non-breaking change which fixes an issue Type: Improvement non-breaking change which improves existing functionality Component: Video Component: Database v21 Omega Feature: Video Versions/Extras labels Jan 10, 2024

CrystalP added this to the Omega 21.0 Beta 3 milestone Jan 10, 2024

ksooo reviewed Jan 10, 2024

View reviewed changes

jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Jan 11, 2024

CrystalP force-pushed the add-extra-validations branch from bc3df0d to 33b4e7c Compare January 11, 2024 09:04

[video] Refactor GetVideoVersionInfo parameters and return type

52ff554

CrystalP force-pushed the add-extra-validations branch from 33b4e7c to 2b30cb9 Compare January 11, 2024 09:10

jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Jan 11, 2024

ksooo reviewed Jan 11, 2024

View reviewed changes

CrystalP force-pushed the add-extra-validations branch from 2b30cb9 to 9328355 Compare January 11, 2024 14:27

CrystalP changed the title ~~[video] Review/adjust the validations and messages in add version/extra~~ [video] Adjust the validations and messages in add version/extra Jan 11, 2024

CrystalP added 3 commits January 11, 2024 11:06

[video] Add success return code to AddVideoExtra

8456711

[video] Add error handling to video version removal

1e40641

[video] Fix incorrect message in version addition

28789fd

CrystalP force-pushed the add-extra-validations branch from 9328355 to 19e7d12 Compare January 11, 2024 16:08

ksooo approved these changes Jan 11, 2024

View reviewed changes

CrystalP force-pushed the add-extra-validations branch from 19e7d12 to 87a10cf Compare January 11, 2024 18:13

CrystalP added 3 commits January 11, 2024 13:43

[video] Revise/align Add version/extra validations and clarify messages

10a1907

[video] Ask the user for confirmation when turning a version into an …

92dd629

…extra or vice-versa

[video] Fix conversion of movies with multiple versions (with user co…

d4f5ff4

…nfirmation)

CrystalP force-pushed the add-extra-validations branch from 87a10cf to 13c7f07 Compare January 11, 2024 18:43

[video] Do not lose extras of the movie added to another via Browse f…

48257fc

…iles

CrystalP force-pushed the add-extra-validations branch from 13c7f07 to 48257fc Compare January 11, 2024 22:35

CrystalP merged commit 39d7860 into xbmc:master Jan 11, 2024
2 checks passed

CrystalP deleted the add-extra-validations branch January 11, 2024 23:24

CrystalP added a commit to CrystalP/xbmc that referenced this pull request Mar 5, 2024

[video] remove unused string 40033

131a29f

Stopped being used after PR xbmc#24477

CrystalP mentioned this pull request Mar 5, 2024

[video] remove unused string #24807

Merged

14 tasks

popcornmix pushed a commit to popcornmix/xbmc that referenced this pull request Apr 8, 2024

[video] remove unused string 40033

f5101ab

Stopped being used after PR xbmc#24477

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[video] Adjust the validations and messages in add version/extra #24477

[video] Adjust the validations and messages in add version/extra #24477

CrystalP commented Jan 10, 2024 •

edited

Loading

ksooo left a comment

ksooo Jan 10, 2024

ksooo Jan 10, 2024

CrystalP Jan 10, 2024

ksooo Jan 11, 2024

CrystalP commented Jan 10, 2024

CrystalP commented Jan 11, 2024

ksooo Jan 11, 2024

CrystalP Jan 11, 2024

CrystalP commented Jan 11, 2024

CrystalP commented Jan 11, 2024

[video] Adjust the validations and messages in add version/extra #24477

[video] Adjust the validations and messages in add version/extra #24477

Conversation

CrystalP commented Jan 10, 2024 • edited Loading

Description

Motivation and context

How has this been tested?

What is the effect on users?

Screenshots (if appropriate):

Types of change

Checklist:

ksooo left a comment

Choose a reason for hiding this comment

ksooo Jan 10, 2024

Choose a reason for hiding this comment

ksooo Jan 10, 2024

Choose a reason for hiding this comment

CrystalP Jan 10, 2024

Choose a reason for hiding this comment

ksooo Jan 11, 2024

Choose a reason for hiding this comment

CrystalP commented Jan 10, 2024

CrystalP commented Jan 11, 2024

ksooo Jan 11, 2024

Choose a reason for hiding this comment

CrystalP Jan 11, 2024

Choose a reason for hiding this comment

CrystalP commented Jan 11, 2024

CrystalP commented Jan 11, 2024

CrystalP commented Jan 10, 2024 •

edited

Loading