Opened 9 years ago

Closed 6 years ago

#9537 closed Bug Report - General (Invalid)

MediaMonitor JumpTo plugin broken in commit ed4d961aed836fade236d006b2797f0c044970de

Reported by: Lawrence Rust <lvr@…> Owned by: stuartm
Priority: minor Milestone: 0.28
Component: MythTV - General Version: Master Head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

In commit ed4d961aed836fade236d006b2797f0c044970de the call to GetMythMainWindow?()->JumpTo?("Main Menu") was moved from MediaMonitor::JumpToMediaHandler? into the plugins MythMusic, MythVideo? & MythGallery.

However, JumpTo?() doesn't complete synchronously, it calls QCoreApplication::postEvent to do the work. When the plugin starts its event loop it finds the jump event and immediately exits without playing the inserted media.

It is necessary to execute the following code after calling JumpTo?():

    GetMythMainWindow()->JumpTo("Main Menu");
    QTime t; t.start();
    while (GetMythMainWindow()->IsExitingToMain() && t.elapsed() < 2000)
        qApp->processEvents(); // Ensure jump is executed before calling handler

Because of this it may be preferable to keep the call to JumpTo?() in MediaMonitor::JumpToMediaHandler? and append the above code rather than replicate it in all plugins.

Change History (11)

comment:1 Changed 9 years ago by stuartm

Milestone: unknown0.25
Owner: set to stuartm
Status: newaccepted

I'd sooner drop the JumpToMediaHandler? behaviour entirely, as I said in the commit, it's obnoxious to jump to the main menu (exiting whatever the user might be doing) on insertion of new media, especially when we don't know at that stage whether the plugin is actually going to act on the event.

comment:2 Changed 9 years ago by stuartm

ok, the reason why JumpTo? is asynchronous is that it's frequently called from outside the UI Thread as is the case here. The callbacks are called outside the UI thread because the mediamonitor (correctly) sits in it's own thread.

I think the best fix is what I proposed the other day on the mailing list, we get rid of the callbacks for plugins and instead use events, if the plugins receive MediaEvents? then they can call JumpTo?() only if required and JumpTo?() can operate synchronously. A gCoreContext->IsUIThread() check in JumpTo?() will handle that neatly.

comment:3 Changed 9 years ago by Lawrence Rust <lvr@…>

Having some kind of autoplay is, I believe, essential. Given that, then the current implementation (with some minor tweaks) works but is clunky and limiting. Allowing a plugin to install some kind of general event handler/filter is more generic but could be a significant code change. One has to look at the cost/benefit analysis. What are the benefits of changing? How much code/time will this add?

The call to JumpTo??("MainMenu??") isn't so terrible. At that point the user has enabled "Jump to plugin" and is expecting Myth to start a compatible handler after inserting new media. Jumping to main menu unravels anything going on, releasing memory and devices, and provides a clean exit point for the new plugin. It's questionable though if one plugin should interrupt another or whether autoplay should be disabled while watching live TV.

I sense that you have an agenda to change this but I haven;t seen the benefits yet. Can you explain?

comment:4 Changed 9 years ago by stuartm

Look at the handlers in the various plugins, there are conditions under which they can be called but nothing will happen, so we exit to the main menu for no good reason. This is even more important since the global setting to 'jump to plugin' was removed leaving just the plugin specific settings.

There are practical reasons for not wanting to jump before we've established whether that's the correct action. Let's say that you have CD autoplay enabled but you're currently on the CD ripping screen, inserting a CD at this point clearly should not exit and start playing instead.

Under the old arrangement we cannot evaluate whether the plugin is going to handle the media without first going to the plugin handler by which time we've already exited to the menu. It's clumsy and not especially useful if we want to make better use of the media monitor in the longer term.

The media monitor is perhaps one of the most underused features of the current code base, it has the potential to be extremely useful but it's limited by the callback structure. The changes I'm suggesting are not that significant in the overall scheme of things and they open the door to a lot of potential benefits. Ultimately it might mean less code than we have now, a leaner and more flexible framework. The changes need not take more than a few days in total. More importantly they would be moving the code in the right direction rather than a hack which only maintains the status quo.

comment:5 Changed 9 years ago by Lawrence Rust <lvr@…>

OK, you have convinced me. But in the interim couldn't we add that small patch to fix what exists?

comment:6 Changed 8 years ago by beirdo

See also #9521

comment:7 Changed 8 years ago by stuartm

Milestone: 0.250.26

comment:8 Changed 8 years ago by stuartm

Type: Bug ReportBug Report - General

comment:9 Changed 7 years ago by Kenni Lund [kenni a kelu dot dk]

Milestone: 0.260.26.1

comment:10 Changed 6 years ago by paulh

Milestone: 0.26.10.28

comment:11 Changed 6 years ago by stuartm

Resolution: Invalid
Status: acceptedclosed
Note: See TracTickets for help on using tickets.