Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add event dashboards and improve dashboard handling #21

Merged
merged 5 commits into from
Mar 22, 2024

Conversation

basyskom-jvoe
Copy link

No description provided.

@basyskom-jvoe basyskom-jvoe requested a review from khaexy March 20, 2024 15:43
@basyskom-jvoe basyskom-jvoe force-pushed the feature/event_dashboards branch from 565b726 to 311dc1c Compare March 21, 2024 07:47
@@ -9,6 +9,7 @@
#define DASHBOARDITEM_H

#include <QObject>
#include <QOpcUaMonitoringParameters>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Überflüssiger include

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -173,8 +186,14 @@ bool DashboardItemModel::isAddItem(uint index) const

void DashboardItemModel::setCurrentIndex(uint index)
{
for (const auto &item : mItems) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Muss das eine for-Schleife sein? Der Index ist doch in dem Moment noch mCurrentIndex oder habe ich da was übersehen?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soweit ich das sehe, gilt das nicht, wenn ein Item weiter vorne in der Liste via removeItem() entfernt wird.

return mEventFilter;
}

QList<QVariantMap> MonitoredItem::lastEvents() const
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funktion kann const reference sein

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

src/backend.cpp Outdated
const auto selectClauses = settings.value(Constants::SettingsKey::EventFilters)
.value<QList<QList<QOpcUaSimpleAttributeOperand>>>();

for (int i = 0; i < nodeIds.size(); ++i) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable i wird schon in der äußeren Schleife verwendet, könnte zu Verwirrungen führen.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -7,14 +7,6 @@

#include "referencemodel.h"

enum Roles : int {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ist das notwendig? Warum sind die enums in die Header-Datei verschoben worden?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Überbleibsel von einem kaputten Implementierungsansatz, ich habe es zurückverschoben

@@ -238,11 +238,226 @@ Rectangle {
}
}

Component {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verschieben geht zwar noch, aber nach dem Verschieben ändert sich die Größe der Kachel nicht mehr bei Events.
Das Verschieben der Event-Dashboards ist in meinen Augen auch nicht sehr intuitiv. Deswegen wäre zu überlegen, ob man das bei Events gar mehr zulässt oder man im Header noch zwei Pfeile zum Verschieben der Reihenfolge einbaut.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich habe die Variante mit den Pfeilen gebaut

If a dashboard is currently not being displayed and
and an event occurs or a value changes, it is marked
with an activity indicator.
@basyskom-jvoe basyskom-jvoe force-pushed the feature/event_dashboards branch from 311dc1c to bd866dd Compare March 21, 2024 15:53
@khaexy khaexy merged commit e1ce887 into develop Mar 22, 2024
3 checks passed
@khaexy khaexy deleted the feature/event_dashboards branch March 22, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants