Architecture redesign

[othieno]

This is mainly a redesign of the architecture with the end-goal of having a more maintanable project. I've modified quite a number of things (see the commit message) so any and all feedback is welcome.

[leafo]

Thanks for the patch. Still looking through it.

Regarding memory leaks, doesn't qt manage memory when you associate a parent. Or are you referring to something else?

I noticed the app now crashes when you quit as well.

[leafo]

I ran master through valgrind via the analyze tab in qtcreator, found one memory leak, not a bunch, but maybe I'm missing something.

[leafo]

I tried searching around some more regarding how people handle allocations in qt applications, it seems like the preferred way is to do dynamic allocation (and passing parent) for anything related to QObject based on all the examples I see, but I couldn't find definitive answer. Thoughts?

In our case, that includes Api, Settings, AppWindow, SettingsWindow and AppController.

I would keep them as pointers to match how other qt apis work unless you see any significant advantages.

[othieno]

@Zetsaika, I am in the wrong here. I seem to have misunderstood the concept behind the secondary window and managed to hardcode it to the settings widget (as it was the only widget i found being used in the secondary window).

@leafo The memory leaks are rather subtle. It is true that Qt automatically manages memory via its parent-child mechanism but like you mentioned, this only works for QObjects. The (potential) memory leaks I was referring to is the creation of a widget's user interface, like this for example. The classic example is If an exception is thrown before a widget is deleted, then that memory is never reclaimed. So to fix this, I created the AbstractView class that relies on a smart pointer to handle the memory.

I run the app through gdb and found that it crashes when trying to free the tray icon (a second time). To fix this, I have made trayIcon an orphaned regular variable so its memory is handled automatically.

Methods can be const overloaded, yes. I have no intelligent reason for having prefixed const member functions with 'const_'. I shall change this.

It is true that a lot of examples rely heavily on Qt's memory handling mechanism, but I also feel like many are inclined to use this feature simply because it exists, without taking its implications into consideration. Here's a good article on Qt Quick performance considerations (transferable to regular Qt) where the chapters on memory allocation (here and here) enunciate some of the problems with dynamic allocations.

My advice would be to use dynamic allocations on ephemeral objects such as the widgets (which I think is what Qt's memory handling mechanism was primarily designed for, I could be wrong).

Personally, I wouldn't lazy-initialize (or dynamically allocate) AppController, Api, Settings or AppWindow since they're guaranteed to exist for as long as the program is running. Since SettingsWindow (SecondaryWindow?) pops in and out occasionally, it's a good candidate for lazy-initialization.

As always, feedback is much appreciated.

[othieno]

While trying to resolve the merge conflicts, it seems that this pull request makes too many changes so I'll try to propose incremental pull requests in the future. Feel free to ignore it.

[leafo]

Wouldn't the exception based memory leak apply to all children of the QObject as well then if you're suggesting that the destructor is not being called (as the QObject constructor is responsible for cleaning up the children). Seems like the memory leak is not just limited to the ui instance variable and is more serious than that. Also if there is an exception preventing the destructor from being called then isn't the issue at the point of the exception, and not at the destructor? (I am not experienced in C++ so tell me if I'm wrong)

I don't think we're abusing the dynamic allocation just because it exists. I think it exists exactly for this purpose because it makes things simple for the developer. I'm not sure of the value of the performance optimizations for these few objects considering I don't even know what qt is doing under the hood. I don't think we should make the optimizations without first identifying that there is a problem and seeing if this is the solution to the problem, seems like premature optimization.

Also feel free to make as many commits as you like in the future without squashing, I don't mind dirtying the history.

Thanks again for the patch

[othieno]

I don't think we should make the optimizations without first identifying that there is a problem and seeing if this is the solution to the problem, seems like premature optimization.

Fair enough. These were definitely premature optimizations so for now I'll stick to feature implementation.

I don't think we're abusing the dynamic allocation just because it exists.

I'm not targeting this project at all but I've seen some Qt projects abuse this feature (I am guilty of this myself) by doing something analogous to this:

int times2(int x)
{
    int result = 0;

    int* two = new int;
    *two = 2;
    result = *two * x;
    delete two;

    return result;
}

So maybe what I'm trying to say is use dynamic allocation responsibly.