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

[POC] OSCQuery #14229

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1296,6 +1296,7 @@ add_library(
src/network/jsonwebtask.cpp
src/network/networktask.cpp
src/network/webtask.cpp
src/osc/query/oscquerydescription.cpp
src/preferences/colorpaletteeditor.cpp
src/preferences/colorpaletteeditor.cpp
src/preferences/colorpaletteeditormodel.cpp
Expand Down
12 changes: 12 additions & 0 deletions src/control/control.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include "control/controlobject.h"
#include "moc_control.cpp"
#include "osc/query/oscquerydescription.h"
#include "util/mutex.h"
#include "util/stat.h"

Expand Down Expand Up @@ -37,6 +38,9 @@ QHash<ConfigKey, QWeakPointer<ControlDoublePrivate>> s_qCOHash
QHash<ConfigKey, ConfigKey> s_qCOAliasHash
GUARDED_BY(s_qCOHashMutex);

OscQueryDescription s_oscQueryDecription
GUARDED_BY(s_qCOHashMutex);

/// is used instead of a nullptr, helps to omit null checks everywhere
QWeakPointer<ControlDoublePrivate> s_pDefaultCO;
} // namespace
Expand Down Expand Up @@ -82,6 +86,7 @@ ControlDoublePrivate::ControlDoublePrivate(
ControlDoublePrivate::~ControlDoublePrivate() {
s_qCOHashMutex.lock();
//qDebug() << "ControlDoublePrivate::s_qCOHash.remove(" << m_key.group << "," << m_key.item << ")";
s_oscQueryDecription.removeControlKey(m_key);
s_qCOHash.remove(m_key);
s_qCOHashMutex.unlock();

Expand Down Expand Up @@ -187,6 +192,7 @@ QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(
const MMutexLocker locker(&s_qCOHashMutex);
//qDebug() << "ControlDoublePrivate::s_qCOHash.insert(" << key.group << "," << key.item << ")";
s_qCOHash.insert(key, pControl);
s_oscQueryDecription.insertControlKey(key);
return pControl;
}

Expand Down Expand Up @@ -215,6 +221,12 @@ QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getDefaultControl() {
return defaultCO;
}

// static
void ControlDoublePrivate::writeOscQueryDescription(QString& oscQueryFileName) {
const MMutexLocker locker(&s_qCOHashMutex);
s_oscQueryDecription.saveToFile(oscQueryFileName);
}

// static
QList<QSharedPointer<ControlDoublePrivate>> ControlDoublePrivate::getAllInstances() {
QList<QSharedPointer<ControlDoublePrivate>> result;
Expand Down
2 changes: 2 additions & 0 deletions src/control/control.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ class ControlDoublePrivate : public QObject {
double defaultValue = kDefaultValue);
static QSharedPointer<ControlDoublePrivate> getDefaultControl();

static void writeOscQueryDescription(QString& oscQueryFileName);

// Returns a list of all existing instances.
static QList<QSharedPointer<ControlDoublePrivate>> getAllInstances();
// Clears all existing instances and returns them as a list.
Expand Down
10 changes: 10 additions & 0 deletions src/dialog/dlgdevelopertools.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ DlgDeveloperTools::DlgDeveloperTools(QWidget* pParent,
&QPushButton::clicked,
this,
&DlgDeveloperTools::slotControlDump);
connect(oscQuerryDump,
&QPushButton::clicked,
this,
&DlgDeveloperTools::slotOscQuerryDump);

// Set up the log search box
connect(logSearch,
Expand Down Expand Up @@ -133,6 +137,12 @@ void DlgDeveloperTools::slotControlDump() {
}
}

void DlgDeveloperTools::slotOscQuerryDump() {
QString oscQueryFileName = m_pConfig->getSettingsPath() +
"/oscquery.json";
ControlDoublePrivate::writeOscQueryDescription(oscQueryFileName);
}

void DlgDeveloperTools::slotLogSearch() {
QString textToFind = logSearch->text();
m_logCursor = logTextView->document()->find(textToFind, m_logCursor);
Expand Down
1 change: 1 addition & 0 deletions src/dialog/dlgdevelopertools.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class DlgDeveloperTools : public QDialog, public Ui::DlgDeveloperTools {
void slotControlSearch(const QString& search);
void slotLogSearch();
void slotControlDump();
void slotOscQuerryDump();
Copy link
Member

Choose a reason for hiding this comment

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

typo is repeated multiple times, please grep for the other occurrences

Suggested change
void slotOscQuerryDump();
void slotOscQueryDump();


private:
UserSettingsPointer m_pConfig;
Expand Down
10 changes: 10 additions & 0 deletions src/dialog/dlgdevelopertoolsdlg.ui
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,16 @@
</attribute>
</widget>
</item>
<item row="0" column="2">
<widget class="QPushButton" name="oscQuerryDump">
<property name="toolTip">
<string>Dumps a OSCQuery description to settings path (e.g. ~/.mixxx)</string>
</property>
<property name="text">
<string>Dump OSCQuerry Description</string>
</property>
</widget>
</item>
</layout>
</widget>
<widget class="QWidget" name="logTab">
Expand Down
191 changes: 191 additions & 0 deletions src/osc/query/oscquerydescription.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
#include "osc/query/oscquerydescription.h"

#include <QDebug>
#include <QFile>
#include <QJsonDocument>
#include <QJsonValue>

#include "preferences/configobject.h"

namespace {

template<typename T>
T take_back(std::vector<T>* pVec) {
T last_element = std::move(pVec->back());
pVec->pop_back();
return last_element;
}

QString toOscAddress(const ConfigKey& key) {
QString groupWithoutBrackets = key.group.mid(1, key.group.size() - 2);
QString oscAddress = groupWithoutBrackets + QChar('/') + key.item;
oscAddress.replace(QChar('['), QChar('('));
oscAddress.replace(QChar(']'), QChar(')'));
oscAddress.replace(QChar('_'), QChar('/'));
return oscAddress;
}

} // namespace

OscQueryDescription::OscQueryDescription()
: m_rootObject({{"DESCRIPTION", "Mixxx OSC root node"},
{"FULL_PATH", "/"},
{"ACCESS", "0"},
{"CONTENTS", QJsonObject()}}) {
}

void OscQueryDescription::addAddress(
const QString& address,
const QString& type,
const QString& access,
const QString& description) {
QStringList pathParts = address.split('/');
QString fullPath;

std::vector<QJsonObject> objects;
objects.reserve(pathParts.size());

// Create a list of QJsonObjects.
// Note: QJsonObject is only temporary helper class for writing a QJsonValue
for (const auto& pathPart : pathParts) {

Check failure on line 50 in src/osc/query/oscquerydescription.cpp

View workflow job for this annotation

GitHub Actions / clazy

c++11 range-loop might detach Qt container (QStringList) [-Wclazy-range-loop-detach]
if (pathPart.isEmpty()) {
continue;
}
fullPath += "/" + pathPart;
if (objects.size()) {
Copy link
Member

Choose a reason for hiding this comment

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

implicit narrowing cast. what does this imply? this code is only executed on first iteration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

QJsonValueRef currentContent = objects.back()["CONTENTS"];
if (currentContent.isObject()) {
objects.push_back(currentContent.toObject());
} else {
objects.emplace_back();
}
} else {
objects.push_back(m_rootObject["CONTENTS"].toObject());
}

QJsonValueRef currentPart = objects.back()[pathPart];
if (currentPart.isObject()) {
objects.push_back(currentPart.toObject());
} else {
objects.emplace_back(QJsonObject({{"ACCESS", "0"}, {"FULL_PATH", fullPath}}));
}
}

// populate
objects.back()["FULL_PATH"] = address;
objects.back()["TYPE"] = type;
objects.back()["ACCESS"] = access;
objects.back()["DESCRIPTION"] = description;

// recreate form the leaves
for (auto it = pathParts.crbegin(); it != pathParts.crend(); ++it) {
objects.back()[*it] = take_back(&objects);
if (objects.size() == 1) {
m_rootObject["CONTENTS"] = take_back(&objects);
break;
}
objects.back()["CONTENTS"] = take_back(&objects);
}
}

void OscQueryDescription::removeAddress(const QString& address) {
QStringList pathParts = address.split('/');

std::vector<QJsonObject> objects;
objects.reserve(pathParts.size());

// Create a list of QJsonObjects.
// Note: QJsonObject is only temporary helper class for writing a QJsonValue
for (const auto& pathPart : pathParts) {

Check failure on line 99 in src/osc/query/oscquerydescription.cpp

View workflow job for this annotation

GitHub Actions / clazy

c++11 range-loop might detach Qt container (QStringList) [-Wclazy-range-loop-detach]
if (pathPart.isEmpty()) {
continue;
}
if (objects.size()) {
QJsonValueRef currentContent = objects.back()["CONTENTS"];
if (currentContent.isObject()) {
objects.push_back(currentContent.toObject());
} else {
objects.back().remove("CONTENTS");
return;
}
} else {
objects.push_back(m_rootObject["CONTENTS"].toObject());
}

QJsonValueRef currentPart = objects.back()[pathPart];
if (currentPart.isObject()) {
objects.push_back(currentPart.toObject());
} else {
objects.back().remove(pathPart);
return;
}
}

if (objects.back().contains("CONTENTS")) {
objects.back().remove("TYPE");
objects.back().remove("DESCRIPTION");
objects.back()["ACCESS"] = "0";
} else {
objects.pop_back();
objects.back().remove(pathParts.takeLast());
if (objects.back().isEmpty()) {
objects.pop_back();
objects.back().remove("CONTENTS");
} else {
objects.back()["CONTENTS"] = take_back(&objects);
}
}

// recreate form the leaves
for (auto it = pathParts.crbegin(); it != pathParts.crend(); ++it) {
if (!objects.back().contains("CONTENTS") && !objects.back().contains("TYPE")) {
objects.pop_back();
objects.back().remove(*it);
} else {
objects.back()[*it] = take_back(&objects);
}
if (objects.size() == 1) {
m_rootObject["CONTENTS"] = take_back(&objects);
break;
}
if (objects.back().isEmpty()) {
objects.pop_back();
objects.back().remove("CONTENTS");
} else {
objects.back()["CONTENTS"] = take_back(&objects);
}
}
}

QString OscQueryDescription::toJsonString() const {
QJsonDocument doc(m_rootObject);
return doc.toJson(QJsonDocument::Indented);
}

bool OscQueryDescription::saveToFile(const QString& filePath) const {
QFile file(filePath);
if (!file.open(QIODevice::WriteOnly)) {
qWarning() << "Failed to open file for writing:" << filePath;
return false;
}

file.write(toJsonString().toUtf8());
file.close();
return true;
}

void OscQueryDescription::insertControlKey(const ConfigKey& key) {
QString address = toOscAddress(key);
addAddress("/cop/" + address, "f", "3", "");
addAddress("/get/cop/" + address, "f", "3", "");
addAddress("/cov/" + address, "f", "3", "");
addAddress("/get/cov/" + address, "f", "3", "");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

what is the "f", "3", "" for?
f = float?

Copy link
Member Author

Choose a reason for hiding this comment

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

All is described here, but we should make the code readable on its own.
https://github.com/Vidvox/OSCQueryProposal
"f" = float
"3" = rw = Read and write.

void OscQueryDescription::removeControlKey(const ConfigKey& key) {
QString address = toOscAddress(key);
removeAddress("/cop/" + address);
removeAddress("/get/cop/" + address);
removeAddress("/cov/" + address);
removeAddress("/get/cov/" + address);
}
30 changes: 30 additions & 0 deletions src/osc/query/oscquerydescription.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
#pragma once

#include <QJsonObject>

class QString;
class QVariant;
class ConfigKey;

class OscQueryDescription {
public:
OscQueryDescription();

void addAddress(
const QString& address,
const QString& type,
const QString& access,
const QString& description);

void removeAddress(const QString& address);
Comment on lines +13 to +19
Copy link
Member

Choose a reason for hiding this comment

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

the implementation of both of these is really hard to understand IMO... Can you try to simplify it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet, it is still premature. Or do you see a low hanging fruit?

Copy link
Member

Choose a reason for hiding this comment

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

I suspect the algorithm would be easier to implement if addAddress and removeAddress would not mutate the json object inplace. Instead it should save it in its own internal datastructure (that is easier to understand than QJsonObject) and only saveToFile serializes that datastructure to a QJsonDocument in one go. I think doing it this way may result in simpler code. wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is probably true. I have started with that but than I realized that the main issue remains. You need to start from n-leaves to built the tree. That's why I started with that here.

We finally need auch a tree to allow wildcard addressing and storing our CO objects.
It could be interesting how this performed compared to QHash using the full address and not support wild card addressing..

@Eve00000 has implemented a code to restore the original ini name of our COs and use the established QHash.

We neet to put all in a pot, steer and have a look.

What do you think?

Copy link
Contributor

@Eve00000 Eve00000 Jan 27, 2025

Choose a reason for hiding this comment

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

HI, just releasing a balloon:
can we, instead of making a hash of the complete collection of CO's implement a sort of 'favourites', I can imagine the user won't use all CO's in /tree/branch/leaf form but only a 'selection as needed'.
If we create a favourites table, we do a lookup of the converted CO once and store the translation in an intermediate hashtable, this will speed up the interaction between /t/b/l to CO.
In this case we do need to implement a regular checkup to control if the translation of the favourites is stil correct.
If this idea is complete stupid, please ignore it but don't shoot me 😄

Copy link
Member

Choose a reason for hiding this comment

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

can we, instead of making a hash of the complete collection of CO's implement a sort of 'favourites', I can imagine the user won't use all CO's in /tree/branch/leaf form but only a 'selection as needed'.

Not sure its worth the complexity. Certainly won't improve performance IIU your proposal correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

you need to start from n-leaves to built the tree.
I don't understand, can you elaborate?

Qt holds a the string like QJsonValue in memory. The QJsonObject is only an editor interface.
If we want a root QJsonValue with {"a":{"b":{"leaf":{}}}} you need to create the leaf first, create an empty b, insert the leaf create an empty a insert be and insert a to the root.

If you want to add something into the leaf you cannot do via the root, because the leaf is read only. You can copy the leaf, alter it and put it back to b. however b is also read only and you need to copy it ... and so forth.
The Root QJsonObject is only one dimensional array [1] with only a, a QJsonValue with all the rest as a string. You need to create QJsonObject for a, then get b, create the QJsonObject for leaf.

Copy link
Member

@Swiftb0y Swiftb0y Jan 27, 2025

Choose a reason for hiding this comment

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

Sure... but thats orthogonal to the design I'm questioning. Why does the class manage the json tree directly instead of keeping its own (easier to manipulate) datastructure and then only build the json tree when actually writing the file? I feel like that would result in a simpler implementation than the current one (whose clarity is limited by the clunky Qt API). Or rather: the limiting factor in terms of code quality seems to the API of the Qt classes, so to improve the code, we limit the amount of code that has to interact with that API. If that code was constrained to the saveToFile member function, the amount of clunky code would be minimized. Does that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had have this also in mind. We will need our own data structure anyway when we decide to allow wild card lockups. However, we always need a similar single branch up ad down crawling when writing the Json File. Since the structure is constant, it is reasonable to keep it in memory and not do it for all 118883 leaves again on every OSCQuery call. Writing the file is only a debug feature for the developers. If we have that code anyway, id does not matter in which function it is.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, I misunderstood then. Yes in case this is called often, it does make sense to do it in place. I thought writing the file was the primary purpose of the class.


void insertControlKey(const ConfigKey& key);
void removeControlKey(const ConfigKey& key);

QString toJsonString() const;

bool saveToFile(const QString& filePath) const;

private:
QJsonObject m_rootObject;
};
Loading