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
Change History
Changed 4 years ago by nigel
- Attachment ctxrestr.patch added
Changed 4 years ago by nigel
- Attachment ctxrestr.2.patch added
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
- Attachment ctxrestr.3.patch added
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
- Attachment ctxrestr.4.patch added
Version that removes all cross-dependencies. Sadly, only works on OS X
Changed 4 years ago by nigel
- Attachment ctxrestr.5.patch added
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
- Attachment ctxrestr.6.patch added
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).
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:
- 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.
- 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?
- 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?
- The initial bootstrapping UI could/should use libmythui.
Changed 4 years ago by anonymous
- Attachment combine-libs.patch added
Inelegant, but simpler than changing the source - combine the 3 libs
Changed 4 years ago by nigel
- Attachment combine-libs.2.patch added
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

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