-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: main
Are you sure you want to change the base?
[POC] OSCQuery #14229
Changes from all commits
0bca791
c626dfb
3de80a5
a8209c0
36821ff
94b91cd
c08c558
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,190 @@ | ||
#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 oscAddress = key.group + key.item; | ||
oscAddress.remove(QChar('[')); | ||
oscAddress.remove(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 : std::as_const(pathParts)) { | ||
if (pathPart.isEmpty()) { | ||
continue; | ||
} | ||
fullPath += "/" + pathPart; | ||
if (objects.size()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 : std::as_const(pathParts)) { | ||
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, "d", "3", ""); | ||
addAddress("/get/cop/" + address, "d", "3", ""); | ||
addAddress("/cov/" + address, "d", "3", ""); | ||
addAddress("/get/cov/" + address, "d", "3", ""); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the "f", "3", "" for? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
void OscQueryDescription::removeControlKey(const ConfigKey& key) { | ||
QString address = toOscAddress(key); | ||
removeAddress("/cop/" + address); | ||
removeAddress("/get/cop/" + address); | ||
removeAddress("/cov/" + address); | ||
removeAddress("/get/cov/" + address); | ||
} |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. @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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HI, just releasing a balloon: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not sure its worth the complexity. Certainly won't improve performance IIU your proposal correctly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Qt holds a the string like QJsonValue in memory. The QJsonObject is only an editor interface. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
}; |
There was a problem hiding this comment.
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