Clean up BCL dialog and remove auth keys #447
Conversation
| QTimer* m_timer; | ||
| QElapsedTimer m_downloadTimer; |
There was a problem hiding this comment.
I'll need to double check this, feels weird that one is a pointer and the other isn't.
There was a problem hiding this comment.
Agree its weird but QElapsedTimer doesn't take a parent in the ctor so it doesn't get cleaned up automatically. I could make it a shared pointer.
| m_checkBox->style()->unpolish(m_checkBox); | ||
| m_checkBox->style()->polish(m_checkBox); |
There was a problem hiding this comment.
This is necessary to redraw the check box indicator (e.g. to show that a component has been downloaded). For some reason setObjectName doesn't change the appearance of the object unless you call this:
| BuildingComponentDialogCentralWidget::~BuildingComponentDialogCentralWidget() { | ||
| m_timer->stop(); | ||
| clearPendingDownloads(false); | ||
| if (m_remoteBCL) { | ||
| m_remoteBCL->waitForComponentDownload(); | ||
| } | ||
| } |
There was a problem hiding this comment.
That seems like a sensible thing the destructor should do.
| m_timer = new QTimer(this); | ||
| connect(m_timer, &QTimer::timeout, this, &BuildingComponentDialogCentralWidget::downloadNextComponent); | ||
| m_timer->start(500); |
There was a problem hiding this comment.
Can you explain this part please?
There was a problem hiding this comment.
I'm confused, even after looking at the code I still don't get how downloadNextComponent is called over and over again?! (maybe it is not...)
There was a problem hiding this comment.
Oh I see. init starts a timer, everytime a download is done, another timer of 500 is started...
There was a problem hiding this comment.
Yeah I had a lot of trouble here. I don't think RemoteBCL actually works to download multiple components, it seems to only work for the first download, I didn't track down why. Also, if multiple RemoteBCL objects download at the same time, the downloads will complete but the entries will not get inserted into LocalBCL. I think LocalBCL is not fully threadsafe. So I had a queue of components to download and used a timer to check the queue every 500 ms.
| m_currentDownload = m_pendingDownloads.front(); | ||
| m_pendingDownloads.pop(); | ||
| if (m_currentDownload->second == "components") { | ||
| m_remoteBCL = std::make_shared<RemoteBCL>(); |
There was a problem hiding this comment.
It pains me a little to see that even if everything is going well (components/measures are downloading all fine) we are destroying then recreating the a RemoteBCL instance, no?
There was a problem hiding this comment.
Agree, but I think this is an issue with RemoteBCL.
| m_progressBar->setVisible(false); | ||
| m_showNewComponents = true; | ||
| } | ||
| m_remoteBCL.reset(); |
There was a problem hiding this comment.
we could check if m_pendingDownloads is empty or something before resetting it.
There was a problem hiding this comment.
When we get here, the current download has completed so we destroy this m_remoteBCL instance. A new m_remoteBCL instance will be created the next time downloadNextComponent is called.
| // show busy progress: we add 1 to both Value and Maximum to immediately indicate to the user that we started doing *something* | ||
| m_progressBar->setValue(m_totalPendingDownloads - m_pendingDownloads.size()); | ||
| m_progressBar->setMinimum(0); | ||
| m_progressBar->setMaximum(m_totalPendingDownloads + 1); |
There was a problem hiding this comment.
@jmarrec nice, I missed that we could do that now
| @@ -1697,7 +1697,7 @@ std::shared_ptr<MainRightColumnController> OSDocument::mainRightColumnController | |||
| } | |||
|
|
|||
| void OSDocument::openMeasuresBclDlg() { | |||
| if (RemoteBCL::isOnline()) { | |||
| if (!RemoteBCL::isOnline()) { | |||
There was a problem hiding this comment.
@jmarrec thanks for catching this
jmarrec
added
component - Backend
component - UI
Enhancement Request
Fixes #408