OpenTails / CRUMPET-Android

The Brains For Your Tail Company Gear!
GNU General Public License v3.0
31 stars 7 forks source link

Auto reconnect #62

Closed ildar-gilmanov closed 5 years ago

ildar-gilmanov commented 5 years ago

Related to https://github.com/MasterTailer/DIGITAiL/issues/61

Also I added SettingsPage.qml.

If we caught an error then we try to reconnect to current device. It should be checked with a real devices.

ildar-gilmanov commented 5 years ago

Moved the autoReconnect field to private class. But I leave the private method at the parent class, because we need that the caller class be a QObject. Another way is inheriting private class from QObject, so I think it is better to have it as is.

And by the way could you tell me why PIMPL is used at this project? It seems it is too superfluously at this project. I see only few reasons to use PIMPL:

  1. In a large libraries, like Qt, where clients can use different headers and binary libs
  2. To decrease compilation time in large projects
  3. To implement implicit sharing

And I do not see that one of the reasons is actual for this project. It is important because using PIMPL increases complexity of the code and development, so we should use it only when we need it.

leinir commented 5 years ago

Moved the autoReconnect field to private class. But I leave the private method at the parent class, because we need that the caller class be a QObject. Another way is inheriting private class from QObject, so I think it is better to have it as is.

That is not really true, we already do this using lambdas in other places (see e.g. the CommandQueue and IdleMode).

And by the way could you tell me why PIMPL is used at this project? It seems it is too superfluously at this project. I see only few reasons to use PIMPL:

  1. In a large libraries, like Qt, where clients can use different headers and binary libs
  2. To decrease compilation time in large projects
  3. To implement implicit sharing

And I do not see that one of the reasons is actual for this project. It is important because using PIMPL increases complexity of the code and development, so we should use it only when we need it.

It also reduces the pollution of the header file, which makes code simpler to follow. While we do not have a binary compatibility promise, what we have is a maintenance promise. We've gone across that before, and while there is no specific technical reason for enforcing the PImpl pattern, what we have is a consistency reason. In short, it makes life easier when all the code follows the same methods of implementation, and unless there is an exceptionally good reason to change it (which will require comments and solid reasoning for the deviation), do not deviate from the conventions of the codebase (it's a part of the code style, which in our case is the kdelibs codestyle, which you also correctly identify as an expansion of the Qt codestyle, and deviation from the codestyle - technical reasons are good, but maintainability and consistency are just as important). Hope that's a bit clearer :)

ildar-gilmanov commented 5 years ago

Yes, I agree with you that we should maintain existed conventions, but I asked about original technical reasons ;)

ildar-gilmanov commented 5 years ago

And I do not use lamba because we may reuse that method several times, and it would be more understandable to keep it as a separated method, not lambda

leinir commented 5 years ago

And I do not use lamba because we may reuse that method several times, and it would be more understandable to keep it as a separated method, not lambda

Don't make the function itself a lamdba, just call it in one - that is what's done in the two classes i mentioned earlier. It would still perform compile time checks (the way your current notation does), without polluting the already quite heavily populated btconnectionmanager class header file with internal implementation details :)

leinir commented 5 years ago

Yes, I agree with you that we should maintain existed conventions, but I asked about original technical reasons ;)

The technical reasons are entirely maintenance and consistency based, rather than on language semantics and compiler details :)