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.

Sign up for GitHub

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

Jump to bottom

fixed: Unable to mark files not in video db yet as watched #17414

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fixed: Unable to mark files not in video db yet as watched #17414

wants to merge 1 commit into from
+10 −1

Conversation

arnova
Copy link
Member

@arnova arnova commented Feb 28, 2020

Strange that I/we didn't notice this before....

@arnova arnova added Type: Fix non-breaking change which fixes an issue Backport: Needed v19 Matrix labels Feb 28, 2020
@arnova arnova added this to the Matrix 19.0-alpha 1 milestone Feb 28, 2020
@arnova arnova mentioned this pull request Feb 28, 2020
Closed
@ksooo
Copy link
Member

ksooo commented Feb 28, 2020

Have you tested that this has no bad side effects, like music items showing "mark as watched" now?

ksooo
ksooo requested changes Feb 29, 2020
View reviewed changes
Copy link
Member

@ksooo ksooo left a comment

Choose a reason for hiding this comment

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

This does not work, because it now shows "mark as watched" for non-video items like songs.

screenshot000

@arnova
Copy link
Member Author

arnova commented Feb 29, 2020

This does not work, because it now shows "mark as watched" for non-video items like songs.

screenshot000

Also updated :-) Should work properly now.

@howie-f
Copy link
Contributor

howie-f commented Feb 29, 2020

tbh i cannot reproduce neither in 18 nor 19

@phunkyfish phunkyfish added the PR Cleanup: Needs Review This PR is good and ready for a review. Someone please review. label Mar 10, 2020
ksooo
ksooo reviewed Apr 25, 2020
View reviewed changes
xbmc/video/ContextMenus.cpp Outdated Show resolved Hide resolved
@DaVukovic DaVukovic mentioned this pull request Apr 26, 2020
Closed
1 task
@DaVukovic
Copy link
Member

DaVukovic commented Apr 26, 2020 •
edited

ignore please. Sorry

@phunkyfish
Copy link
Contributor

@arnova could you provide repro instructions? How exactly do access a video item so it's not in the VideoDB yet? Or is it simply a video item in a source prior to a library scan?

@arnova
Copy link
Member Author

arnova commented Apr 27, 2020

@arnova could you provide repro instructions? How exactly do access a video item so it's not in the VideoDB yet? Or is it simply a video item in a source prior to a library scan?

@phunkyfish : Browse to a folder in the files section which hasn't been opened before with "extract flags/thumbsnails" enabled is that will automatically add the file item to the video db. And RMB on one of the files there.

@ksooo : It just came to mind that enabling this logic for "Mark as unwatched" makes no sense since any item that is considered watched must already be in the db. Another question: you mentioned that it shouldn't be in IsVisible() but atm I don't see any other location where this should go then. Mind elaborating? Wouldn't it be better to drop is in IsVisible() for now and fixing it up after the refactor?

@phunkyfish
Copy link
Contributor

@arnova woohoo, now I see, thanks!

phunkyfish
phunkyfish requested changes Apr 27, 2020
View reviewed changes
xbmc/video/ContextMenus.cpp
return false;

const int currentWindow = CServiceBroker::GetGUI()->GetWindowManager().GetActiveWindow();
if (currentWindow == WINDOW_VIDEO_NAV))
Copy link
Contributor

Choose a reason for hiding this comment

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

return currentWindow == WINDOW_VIDEO_NAV;

@ksooo
Copy link
Member

ksooo commented Apr 27, 2020

I will not stop saying: Making the visible decision based on the active window is a hack hack hack!

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label May 9, 2020
@jenkins4kodi
Copy link
Contributor

@arnova this needs a rebase

@DaveTBlake
Copy link
Member

I will not stop saying: Making the visible decision based on the active window is a hack hack hack!

Then @ksooo how should we prevent items on the context menu that are not wanted on certain windows? Seems to me that sometimes the window is also part of the context

@ksooo
Copy link
Member

ksooo commented Jul 15, 2020

Yes, the window can be part of the context, but not for watched/unwatched as watched state is completely unrelated to any window.

@ksooo
Copy link
Member

ksooo commented Jul 15, 2020

BTW: doesn't fix #18023 fix this issue? Not sure about this.

@arnova
Copy link
Member Author

arnova commented Jul 18, 2020

BTW: doesn't fix #18023 fix this issue? Not sure about this.

I don't see how that would fix this issue.

@ksooo
Copy link
Member

ksooo commented Jul 18, 2020

content-less == no db entry?

@arnova
Copy link
Member Author

arnova commented Jul 19, 2020

content-less == no db entry?

Content-less doesn't mean item can't be in db. Furthermore I believe this change only "allows" marking/unmarking again for content-less sources with items that are also in db rather than enabling it for items NOT in the db.

@DaveTBlake
Copy link
Member

@arnova understand this both got bogged down and you have been unable to get back to it. Bumping to v20 since I doubt you will be able to complete for v19. However if you are able to look at this for v19 (as a fix it will be welcome at any point in the cycle) please do and change labels back.

@DaveTBlake DaveTBlake added v20 Nexus and removed Backport: Done PR Cleanup: Needs Review This PR is good and ready for a review. Someone please review. v19 Matrix labels Oct 29, 2020
@DaveTBlake DaveTBlake removed this from the Matrix 19.0-alpha 3 milestone Oct 29, 2020
@arnova
Copy link
Member Author

arnova commented Mar 29, 2021

So any ideas how to proceed with this? @ksooo / @DaveTBlake / ... ?

@fuzzard fuzzard removed the v21 Omega label Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ksooo ksooo ksooo requested changes

@phunkyfish phunkyfish phunkyfish requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Rebase needed PR that does not apply/merge cleanly to current base branch Type: Fix non-breaking change which fixes an issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@arnova @ksooo @howie-f @DaVukovic @phunkyfish @jenkins4kodi @DaveTBlake @fuzzard