-
-
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
Smarties (or SmartList or SmartCrate or QueryCrate or QueryList or ...) open for feedback. #14182
base: main
Are you sure you want to change the base?
Conversation
@ronso0 As a draft can't be used to merge and a draft does not create builds I changed the status again. |
I'm not sure what you mean by that. The point of making a PR a draft is to signal to other contributor that this PR is not yet ready for review, and eventually merge. Once your PR reach maturity (the code reach a certain quality, some testing is provided and/or all the CI checks are passing), you may mark the PR as ready, so the core team knows when to start focusing on getting work merged
Drafts do create builds I'm moving your branch back to draft. Once the PR has reached the maturity criteria mentionned above, feel free to mark it ready so the team can start the review process and merge. Please don't do it beofre as this could lead to unwanted notification spam, or unneded effort on reviewing not ready work. Also, am I correct in assuming that this is superseeding the PR #13978? If so, could we close it? Am I correct in assuming that |
Hi, as a draft people will not try it, it needs some review. |
if draft means 'no review request ' than this is certainly not a draft. |
That's not quite right, people will try even draft PR. I have many draft PRs that still are getting tested. It mainly boils down to interest, which is unrelated to PR status.
I think there is confusion here - a review in the context of PR means a reviewal of the code (the implementation details, the design, the style...). It also include testing but isn't strictly reserved to it, meaning that making a PR ready just to get testing is not really respectful of the other contributor's time. I think you've done the right think by calling for feedback on Zulip (and potentially somewhere else), but as they say in English "you can lead a horse to water, but you can't make him drink" When it comes to review readiness, note that contributors will usually prefer something smaller (see the stem feature or the rendergraph, which was delivered as multiple small PR). It's not easy to motivate core members to review a 15k lines of C++ ;)
Ah yeah, of course. Now that you are saying it, I realise it was already clear to me. Sorry for the noise on that one! |
I saw that too once or twice. Guess it's Github bug because lately it just worked for my draft PRs. |
Note that Github Actions have been quite unstable since the beginning of the year (see status page and history) so definitely could be that! Remember that actions were built after the Github acquisition by Microsoft, so it is expected to be buggy and unstable ;) |
@Eve00000 I haven't had a deeper look at this feature yet (though it looks useful). I just would advise against the naming |
"Smart Crates" would make most sense for the user-facing designation of this feature).
|
newSmarties.setCondition6Combiner(") END"); | ||
newSmarties.setCondition5Combiner(orCombiner ? ") OR (" : ") AND ("); | ||
break; | ||
case 7: | ||
newSmarties.setCondition7Field(term); | ||
newSmarties.setCondition7Operator(operatorType); | ||
newSmarties.setCondition7Value(value); | ||
newSmarties.setCondition7Combiner(") END"); | ||
newSmarties.setCondition6Combiner(orCombiner ? ") OR (" : ") AND ("); | ||
break; | ||
case 8: | ||
newSmarties.setCondition8Field(term); | ||
newSmarties.setCondition8Operator(operatorType); | ||
newSmarties.setCondition8Value(value); | ||
newSmarties.setCondition8Combiner(") END"); | ||
newSmarties.setCondition7Combiner(orCombiner ? ") OR (" : ") AND ("); | ||
break; | ||
case 9: | ||
newSmarties.setCondition9Field(term); | ||
newSmarties.setCondition9Operator(operatorType); | ||
newSmarties.setCondition9Value(value); | ||
newSmarties.setCondition9Combiner(") END"); | ||
newSmarties.setCondition8Combiner(orCombiner ? ") OR (" : ") AND ("); | ||
break; | ||
case 10: | ||
newSmarties.setCondition10Field(term); | ||
newSmarties.setCondition10Operator(operatorType); | ||
newSmarties.setCondition10Value(value); | ||
newSmarties.setCondition10Combiner(") END"); | ||
newSmarties.setCondition9Combiner(orCombiner ? ") OR (" : ") AND ("); | ||
break; | ||
case 11: | ||
newSmarties.setCondition11Field(term); | ||
newSmarties.setCondition11Operator(operatorType); | ||
newSmarties.setCondition11Value(value); | ||
newSmarties.setCondition11Combiner(") END"); | ||
newSmarties.setCondition10Combiner(orCombiner ? ") OR (" : ") AND ("); | ||
break; | ||
case 12: | ||
newSmarties.setCondition12Field(term); | ||
newSmarties.setCondition12Operator(operatorType); | ||
newSmarties.setCondition12Value(value); | ||
newSmarties.setCondition12Combiner(") END"); | ||
newSmarties.setCondition11Combiner(orCombiner ? ") OR (" : ") AND ("); | ||
break; | ||
} | ||
|
||
if (sDebug) { | ||
qDebug() << "[SMARTIES] [NEW SMARTIES FROM SEARCH] -> " | ||
"Detected Term:" | ||
<< term << ", Value:" << value; | ||
} | ||
++conditionIndex; | ||
} | ||
} | ||
|
||
// what if no terms (field) were in the input .... | ||
if (!foundTerms) { | ||
if (sDebug) { | ||
qDebug() << "[SMARTIES] [NEW SMARTIES FROM SEARCH] -> No term " | ||
"detected, setting noTermValue:" | ||
<< text.trimmed(); | ||
} | ||
cleanedText.replace("\"", ""); | ||
newSmarties.setSearchSql(newName); | ||
newSmarties.setCondition1Field("artist"); | ||
newSmarties.setCondition1Operator("contains"); | ||
newSmarties.setCondition1Value(cleanedText); | ||
newSmarties.setCondition1Combiner(") OR ("); | ||
newSmarties.setCondition2Field("album_artist"); | ||
newSmarties.setCondition2Operator("contains"); | ||
newSmarties.setCondition2Value(cleanedText); | ||
newSmarties.setCondition2Combiner(") OR ("); | ||
newSmarties.setCondition3Field("title"); | ||
newSmarties.setCondition3Operator("contains"); | ||
newSmarties.setCondition3Value(cleanedText); | ||
newSmarties.setCondition3Combiner(") OR ("); | ||
newSmarties.setCondition4Field("album"); | ||
newSmarties.setCondition4Operator("contains"); | ||
newSmarties.setCondition4Value(cleanedText); | ||
newSmarties.setCondition4Combiner(") OR ("); | ||
newSmarties.setCondition5Field("genre"); | ||
newSmarties.setCondition5Operator("contains"); | ||
newSmarties.setCondition5Value(cleanedText); | ||
newSmarties.setCondition5Combiner(") OR ("); | ||
newSmarties.setCondition6Field("composer"); | ||
newSmarties.setCondition6Operator("contains"); | ||
newSmarties.setCondition6Value(cleanedText); | ||
newSmarties.setCondition6Combiner(") OR ("); | ||
newSmarties.setCondition7Field("comment"); | ||
newSmarties.setCondition7Operator("contains"); | ||
newSmarties.setCondition7Value(cleanedText); | ||
newSmarties.setCondition7Combiner(") END"); | ||
} | ||
|
||
newSmarties.setName(std::move(newName)); | ||
DEBUG_ASSERT(newSmarties.hasName()); | ||
newSmarties.setSearchInput(text); | ||
newSmarties.setSearchSql(text); | ||
break; |
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.
Instead of rolling your own parsing method here, I would suggest instead reusing SearchQueryParser::parseQuery
which gives you an AST that can be analyzed further.
I would even go as far as suggesting to get rid of ConditionNField/Value/Operator/Combiner
completely, and instead just store the search query directly. The search query syntax can be extended to support all the features needed (like AND/OR).
The table-like editing of the conditions can in principle be implemented by using SearchQueryParser::parseQuery
to turn the text into a tree of query nodes. The tree can then be manipulated using the existing UI, and then serialized back into textual form.
Not all search query may be representable in this tabular form (namely if they contain deeply nested AND/OR clauses). We could simply fall back to just allow editing the search query text in those cases - kind of like JIRA does for Basic vs. Advanced Search.
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.
The fields are needed to create the form (dialog) and the relations between conditions - operators - calues - combiners,
Believe me, I tried first in a simplier way, by converting the sql (see sql field), but .. that was an easy WTF-reason.
So the fields - dialog are the base.
I've spend already a lot of time in converting the parsequery (like with regex etc)... something's blocking in my mind.
I think it would be easier to convert it in the queryparse and pass it as a qvariantlist to the helper
or direct to smartiesstorrage
QString SmartiesTableModel::buildWhereClause(const QVariantList& smartiesData) { | ||
QString whereClause = "("; | ||
bool hasConditions = false; | ||
|
||
QStringList combinerOptions = {") END", "AND", "OR", ") AND (", ") OR ("}; | ||
// Assuming searchValue is at index 6 (search_input) | ||
const QString& searchValue = smartiesData[6].toString(); // search_input | ||
// QString condition; | ||
// Assuming searchValue is at index 7 (search_sql) | ||
// QString searchValue = smartiesData[7].toString(); // seatch_sql | ||
|
||
for (int i = 1; i <= 12; ++i) { | ||
int baseIndex = 8 + (i - 1) * 4; // Adjusting for the correct index in smartiesData | ||
|
||
const QString& field = smartiesData[baseIndex].toString(); | ||
const QString& op = smartiesData[baseIndex + 1].toString(); | ||
const QString& value = smartiesData[baseIndex + 2].toString(); | ||
// QString combiner = smartiesData[baseIndex + 3].toString(); | ||
|
||
// begin build condition | ||
// function moved to smartiesfunctions.h to share it with dlgsmartiesinfo to create preview | ||
const QString& condition = buildCondition(field, op, value); | ||
|
||
// end build condition | ||
if (condition != "") { | ||
hasConditions = true; | ||
whereClause += condition; | ||
if (i < 12 && combinerOptions.contains(smartiesData[baseIndex + 3].toString())) { | ||
if (smartiesData[baseIndex + 3].toString() == ") END") { | ||
whereClause += ")"; | ||
} else if ((smartiesData[baseIndex + 3].toString() == "AND") || | ||
(smartiesData[baseIndex + 3].toString() == "OR")) { | ||
whereClause += " " + smartiesData[baseIndex + 3].toString() + " "; | ||
} else { | ||
whereClause += smartiesData[baseIndex + 3].toString() + " "; | ||
} | ||
} | ||
} | ||
} | ||
|
||
if (!hasConditions) { | ||
whereClause += QStringLiteral( | ||
"library.artist LIKE '%%1%' OR " | ||
"library.title LIKE '%%1%' OR " | ||
"library.album LIKE '%%1%' OR " | ||
"library.album_artist LIKE '%%1%' OR " | ||
"library.composer LIKE '%%1%' OR " | ||
"library.genre LIKE '%%1%' OR " | ||
"library.comment LIKE '%%1%')") | ||
.arg(searchValue); | ||
} | ||
|
||
// whereClause += ")"; | ||
|
||
if (sDebug) { | ||
qDebug() << "[SMARTIESTABLEMODEL] [GETWHERECLAUSEFORSMARTIES] " | ||
"[CONSTRUCT SQL] -> Constructed WHERE clause:" | ||
<< whereClause; | ||
} | ||
return whereClause; | ||
} |
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.
Instead of rolling your own SQL query builder logic here, I would suggest reusing QueryNode::toSql()
which does the same thing.
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.
smarties is more extensive : conditions with playlists / crates / historylists, conditions with pre-formatted values and operators.
did you test it? (if you tested it you would have noticed)
There is no method for parsing these operators from a search query yet. At this stage, they are present purely for supporting all fields and operators already present in mixxxdj#14182.
Smarties
some remarks:
I read #5575 carefully and tried to follow some ideas from that conversation.
smarties are crates with other capabilities : No autoDJcrate, no adding tracks, no export (make crate of the smarties)...
smarties can be 'live' and 'cached' : when the smarties are locked they are cached, else the search is executed when selected (but this goes fast, I'm testing with 13000 tracks table in Virtual Machine, it's just a wink). Cached = tracks in smarties_tracks
-> when title / name of track is changed and the track is no longer conform with the condition the track continues to stay in the smarties until the smarties is unlocked.
in querysearch ienter your seartch, push the white square and your search becomes a a smarties.
click with right mousebutton on a smarties -> edit -> 12 conditions, all track fields with an operator and an value-input
-> Smarties are SMART
all meaningful fields of the library can be selected, with matching operators.
it's possible to use a crate / playlist or historylist in a condition (dropdown)
each condition needs to end with AND / OR vombined with ( )
the last condition needs to end with ) END.
conditions can be switched moved up / down / inserted / deleted. only 12 conditions possible.
with apply the query is shown as it would be executed
before are smarties query is saved it's checked -> green rectanhle ok, red = problem
smarties are updated while updating the conditions -> apply
I added the 'grouped' logic as in 'grouped crates"
please test and send me feedback
NOTE: there are still a lot of 'leftovers' from conversion of crates to smarties, things that might be used in the future (I'm still in dubio).
20241026.Eve.Mixxx.Smarties.mp4
20241026.Eve.Mixxx.Smarties.mp4
Added (2025/02/01) :
![afbeelding](https://private-user-images.githubusercontent.com/159239887/408813578-27288d07-790a-4428-8404-9bd8c4ffa32e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk3MjI4NDMsIm5iZiI6MTczOTcyMjU0MywicGF0aCI6Ii8xNTkyMzk4ODcvNDA4ODEzNTc4LTI3Mjg4ZDA3LTc5MGEtNDQyOC04NDA0LTliZDhjNGZmYTMyZS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjE2JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxNlQxNjE1NDNaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1kMzkzMTUxOGNiZmIzMWNjYjhmZjVjZWMzNjRjOTdhZTkyZTg4NmQ4NDRhNTFjZTU4NDYzZTk0MDU4ZGJmOTdjJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.Igy_JRgViBZEtvgBUknRqVp7Hoj8_m0ycL1J7vECx-g)
![afbeelding](https://private-user-images.githubusercontent.com/159239887/408813622-87fac03c-72de-4535-bd5f-eb5acb23af15.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk3MjI4NDMsIm5iZiI6MTczOTcyMjU0MywicGF0aCI6Ii8xNTkyMzk4ODcvNDA4ODEzNjIyLTg3ZmFjMDNjLTcyZGUtNDUzNS1iZDVmLWViNWFjYjIzYWYxNS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjE2JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxNlQxNjE1NDNaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT02ZGE1YTEzZDI1YzQzMDFhOWZjNDMxYmY3M2E2MmZlMmJkZDJhMDA4NjM4OWRiN2QzOGFiZTk5MTdkNmY3MzE2JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.cojO0Svrou_k0n5BjDspxEXtvxQ74-WYEbfPmsfbMwA)
"track" - "is a member of"/"is not a member of" - "all playlists"/"all crates"/"all historylists"