mzanetti / kodimote

Kodimote is a remote control application for Kodi
GNU General Public License v3.0
20 stars 9 forks source link

Introduce C++11 #21

Open mzanetti opened 9 years ago

mzanetti commented 9 years ago

Just a reminder to keep an eye on Jolla introducing GCC 4.8 in Sailfish OS so we can turn on C++11. According to their roadmap it's said as being worked on currently: https://lists.sailfishos.org/pipermail/devel/2015-January/005512.html

RobertMe commented 9 years ago

If @tempura-san is going to maintain the generic/N900 and Harmattan version then this wouldn't be possible? As those (or at least Harmattan SDK) use an old GCC version and Qt 4.8 and thus don't support function pointer based QObject::connect.

tempura-san commented 9 years ago

Well to be honest the question is: does it make sense to maintain backwards compatibilty if it would introduce major drawbacks in future implementations? Currently I do not have enough spare time to take up the maintaining seriously. I am always more confident with adding small patches here and there on an irregular basis. As far as I understand using the more recent features will irrevokably break reasonable maintainability of the older ports (generic, harmattan and plasmoid) - so unfortunately I suppose it would be wiser to drop them. The library is the binding part between the different UIs - which also offer different levels of usage of the library's possibilities (at least w.r.t. to the older UIs, which were not adapted). So without finding a way to unify the UIs in a way, so that they could reuse code and only add a very small amount of UI glue necessary for each platform - which is not possible as far as I know - maintaining the separate ports is quite cumbersome. Reengineering your UIs in order to implement features in older parts may not be the best and leanest way to go.

Last but not least it would reduce the number of translation lines significantly. ;)

RobertMe commented 9 years ago

This is mostly about using some new C++11 and Qt 5 features. It's not like we need to completely break the compilation of the old ports to add new features to the app itself. But these new C++11 and Qt5 features help us to find bugs earlier (break compilation, instead of something which doesn't work as expected, sometimes with a runtime error being shown/logged, but sometimes even that isn't the case)

For example:

class Base
{
  virtual void doSomething() { }
}

class Derived : public Base
{
  void doSomething(int foo) override { }
}

This won't compile on C++11 as the Derived::doSomething doesn't override anything. But as the override keyword is new we can't use it. In which case the code does compile, but when you do something like (new Derirved())->doSomething() it will actually invoke Base::doSomething, while you expect it to invoke Derived::doSomething (which is unexpected, hard to find, and might lead to bugs).

And the same goes for connecting signals and slots:

//doesn't compile
connect(this, &Foo::nonExistantSignal, this, &Foo::nonExistantSlot);

//does compile, but fails at runtime
connect(this, SIGNAL(nonExistantSignal()), this, SLOT(nonExistantSlot()));

So in both these cases the Qt5 and/or C++11 features help us to discover errors earlier/during compilation.

Unifying the UIs also doesn't solve this problem. As you still need to compile for the different platforms. And we thus still can't use new Qt5 and/or C++11 features (what this issue is about).

tempura-san commented 9 years ago

The point about unifiying the UIs was more about being able to spread features among UIs with less effort. Currently they are maintained separately, so each new features has to be ported separately and also adopted to the platform's possibilities. Of course this has nothing to do with the library switching to Qt5/C++11. :)

RobertMe commented 9 years ago

Ok, I misinterpreted it then.

But unifying the UI indeed isn't possible. But I think most features would work on the other platforms without too much effort. For example adding support for browsing music and video addons, and playlists, didn't require a single line of code change in any platform. But to be fair, that's browsing and only uses the BrowserPage and the enterItem() logic. But still, when the app for a platform is done/feature complete, it shouldn't require that much effort to keep everything up to date (it's maybe only 10% of the total work being done on a feature, and most changes are more or less the same on all platforms, so based on the initial implementation it's just a matter of "copy/pasting" some code and modifying it a bit for the target platform).

But of course, generic/N900 and N9 are missing some features, maybe have some compile errors (which you already fixed for generic :)) etc. But I guess that's mostly because they have been unmaintained for a while. Not because of some huge changes in the lib.