Modify

Ticket #4264 (closed task: fixed)

Opened 4 years ago

Last modified 4 years ago

MythContext restructure

Reported by: nigel Owned by: nigel
Priority: minor Milestone: 0.22
Component: mythtv Version: head
Severity: medium Keywords:
Cc: Ticket locked: no

Description (last modified by nigel) (diff)

Some packagers are having problems building MythTV now that there is another circular dependency in the libraries. libmyth uses libmythupnp, which in turn depends on libmyth.

There are some hacks to get past this, but the long-term solution is to remove this dependency (and the one between libmyth and libmythui).

My current proposal is to put some of the gnarlier MythContextPrivate stuff into another myth library (e.g. mythbase). This would pull in the MythContext code and the UPnP stuff, and be called from (linked against) the myth programs.

Attachments

ctxrestr.patch (149.3 KB) - added by nigel 4 years ago.
Move all UI and UPnP stuff from mythcontext.cpp into libmythui
ctxrestr.2.patch (145.9 KB) - added by nigel 4 years ago.
Move stuff into libmythtv instead, forget about casts when deleting gContext
ctxrestr.3.patch (154.9 KB) - added by nigel 4 years ago.
More obvious MythContext class hierarchy, global for database/WOL params, removed un-needed functions & variables
ctxrestr.4.patch (171.9 KB) - added by nigel 4 years ago.
Version that removes all cross-dependencies. Sadly, only works on OS X
ctxrestr.5.patch (172.4 KB) - added by nigel 4 years ago.
Remove all library cross-dependencies
ctxrestr.6.patch (177.5 KB) - added by nigel 4 years ago.
Remove all x-deps, patch plugins to build
ctxrestr.7.patch (178.1 KB) - added by nigel 4 years ago.
Patch against r15333
ctxrestr.9.patch (168.4 KB) - added by nigel 4 years ago.
Patch against r15462
combine-libs.patch (11.0 KB) - added by anonymous 4 years ago.
Inelegant, but simpler than changing the source - combine the 3 libs
combine-libs.2.patch (10.8 KB) - added by nigel 4 years ago.
MinGW library name and install target fixes

Change History

Changed 4 years ago by nigel

Move all UI and UPnP stuff from mythcontext.cpp into libmythui

Changed 4 years ago by nigel

Move stuff into libmythtv instead, forget about casts when deleting gContext

comment:1 Changed 4 years ago by nigel

There is one more change I might implement - to make MythContextUI a direct subclass of MythContext, and then MythContextUIGUI/TTY subclasses of that. This way would be more natural, and while it exposes some more "private" stuff un-necessarily, it prevents many nasty m_parent->blah() calls in MythContextUIPrivate

Changed 4 years ago by nigel

More obvious MythContext class hierarchy, global for database/WOL params, removed un-needed functions & variables

comment:2 follow-up: ↓ 6 Changed 4 years ago by nigel

It looks like the behaviour of this on OS X is very different to Linux.

On OS X, it uses the vtable of the sub-classes, so gContext->Blah() calls MythContextGUI::Blah()

On Linux, it seems to always have the base class's type. There is probaly some C++ qualifier to get around this, but it is beyond my current understanding of the language - I barely understand inheritance, let alone inheritance with pure-virtual methods :-)

I have a patch that eliminates all cross-dependencies on OS X, but to get it working on Linux I might have to change this all again :-(

Changed 4 years ago by nigel

Version that removes all cross-dependencies. Sadly, only works on OS X

Changed 4 years ago by nigel

Remove all library cross-dependencies

comment:3 Changed 4 years ago by nigel

OK. I think that is now correct for mythtv. The plugins will have to wait for a while. (unless someone else wants to make those changes for me - it should simple be a few extra header files exported by inc.files, change GetDatabaseParams?() to gDBparams, and minor changes in main.cpp for MythContextTTY initialisation)

comment:4 Changed 4 years ago by Doug Goldstein <cardoe@…>

I am happy that upstream is finally doing something about this. This has been an issue for as far back as 0.17 or maybe earlier. I've opened bugs before and proposed patches before but was told the way I had depends was not how you guys wanted them to work.

If you're looking for 0.20-fixes patches, I've got them in Gentoo's CVS.

Changed 4 years ago by nigel

Remove all x-deps, patch plugins to build

comment:5 Changed 4 years ago by nigel

Patch 6 should do everything. Note that the patch paths are from a directory above mythtv and mythplugins

comment:6 in reply to: ↑ 2 Changed 4 years ago by bullestock@…

Replying to nigel:

It looks like the behaviour of this on OS X is very different to Linux.

On OS X, it uses the vtable of the sub-classes, so gContext->Blah() calls MythContextGUI::Blah()

On Linux, it seems to always have the base class's type.

Does the base class have a virtual destructor?

comment:7 Changed 4 years ago by nigel

At the time it didn't. I got around the problem by making the relevant base methods pure virtual (i.e. virtual type Blah() = 0; ). It now does have a virt. dest'r. Is this a requirement? (my C++ skills are still limited, which I want lots of review/testing of this).

Changed 4 years ago by nigel

Patch against r15333

Changed 4 years ago by nigel

Patch against r15462

comment:8 Changed 4 years ago by nigel

  • Description modified (diff)

The circular dependencies have hopefully been worked around in [15478] and [15481]. That will give me more time to develop this some more (maybe add soon after 0.21). I am currently not happy with:

  1. MythContext becoming an un-usable class (due to some pure virtual methods). It is unlikely that anyone would want to instantiate a MythContext and then set the database stuff manually, but for a quick test program, it is feasible.
  2. The structure. Having it in libmythtv means that everything now depends on libmythtv instead of libmyth. That is maybe an unavoidable cost of having UPnP magic, and a UI for choosing a backend or editing the DB parameters, but there is maybe a better compromise?
  3. I could (should?) move all the GUI stuff into the sub-classes. A text program will never need to lookup the window size, so why should it be in the base MythContext class?
  4. The initial bootstrapping UI could/should use libmythui.

comment:9 Changed 4 years ago by nigel

  • Milestone changed from 0.21 to 0.22

Changed 4 years ago by anonymous

Inelegant, but simpler than changing the source - combine the 3 libs

Changed 4 years ago by nigel

MinGW library name and install target fixes

comment:10 Changed 4 years ago by ijr

  • Status changed from new to closed
  • Resolution set to fixed

This was fixed by r17650

View

Add a comment

Modify Ticket

Action
as closed
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.