Opened 11 years ago

Closed 11 years ago

#4264 closed task (fixed)

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)

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 (10)

ctxrestr.patch (149.3 KB) - added by Nigel 11 years ago.
Move all UI and UPnP stuff from mythcontext.cpp into libmythui
ctxrestr.2.patch (145.9 KB) - added by Nigel 11 years ago.
Move stuff into libmythtv instead, forget about casts when deleting gContext
ctxrestr.3.patch (154.9 KB) - added by Nigel 11 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 11 years ago.
Version that removes all cross-dependencies. Sadly, only works on OS X
ctxrestr.5.patch (172.4 KB) - added by Nigel 11 years ago.
Remove all library cross-dependencies
ctxrestr.6.patch (177.5 KB) - added by Nigel 11 years ago.
Remove all x-deps, patch plugins to build
ctxrestr.7.patch (178.1 KB) - added by Nigel 11 years ago.
Patch against r15333
ctxrestr.9.patch (168.4 KB) - added by Nigel 11 years ago.
Patch against r15462
combine-libs.patch (11.0 KB) - added by anonymous 11 years ago.
Inelegant, but simpler than changing the source - combine the 3 libs
combine-libs.2.patch (10.8 KB) - added by Nigel 11 years ago.
MinGW library name and install target fixes

Download all attachments as: .zip

Change History (20)

Changed 11 years ago by Nigel

Attachment: ctxrestr.patch added

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

Changed 11 years ago by Nigel

Attachment: ctxrestr.2.patch added

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

comment:1 Changed 11 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 11 years ago by Nigel

Attachment: ctxrestr.3.patch added

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

comment:2 Changed 11 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 11 years ago by Nigel

Attachment: ctxrestr.4.patch added

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

Changed 11 years ago by Nigel

Attachment: ctxrestr.5.patch added

Remove all library cross-dependencies

comment:3 Changed 11 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 11 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 11 years ago by Nigel

Attachment: ctxrestr.6.patch added

Remove all x-deps, patch plugins to build

comment:5 Changed 11 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 11 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 11 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 11 years ago by Nigel

Attachment: ctxrestr.7.patch added

Patch against r15333

Changed 11 years ago by Nigel

Attachment: ctxrestr.9.patch added

Patch against r15462

comment:8 Changed 11 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 11 years ago by Nigel

Milestone: 0.210.22

Changed 11 years ago by anonymous

Attachment: combine-libs.patch added

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

Changed 11 years ago by Nigel

Attachment: combine-libs.2.patch added

MinGW library name and install target fixes

comment:10 Changed 11 years ago by Isaac Richards

Resolution: fixed
Status: newclosed

This was fixed by r17650

Note: See TracTickets for help on using tickets.