Humanreadable duration search enabled #186

kBashar · 2014-03-08T03:33:28Z

Now MIXXX can support duration search for following style
2:30
2m30s
150

i have added test code with it.

kain88-de · 2014-03-08T10:34:13Z

src/library/searchquery.cpp

+            //qDebug()<<dbl<<durationHumanReadable.at(i);
+                tempStr = "";
+        }
+        else{


else should be on the some line as the closing bracket

kain88-de · 2014-03-08T10:38:26Z

there are still some issues with your coding style. See the wiki and surrounding code for examples

http://www.mixxx.org/wiki/doku.php/coding_guidelines

Can you please fix those. Also you commit messages can/should be a bit more descriptive. See the following for example.

http://robots.thoughtbot.com/5-useful-tips-for-a-better-commit-message
http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

you can change commit messages with git rebase. checkout ry's tutorial for help
http://rypress.com/tutorials/git/rebasing.html

I'll look over the rest in the afternoon

kBashar · 2014-03-08T14:25:45Z

I'm really sorry for my ignorance.
I've modified the code style.
Before making any commit i need to be clear, will "git rebase " modify my already pushed commit?

kain88-de · 2014-03-08T14:35:10Z

Don't worry. Code reviews are there to catch stuff like this.

yeah rebase will rewrite your history. That is ok here. Your are the only one working on this branch and github will automatically update any changes in your branch in this PR. I'd recommend that you experiment a bit with rebase, the rypress tutorial is interactivly!!. If you don't feel comfortable doing it I can do it when I merge this with master

But to be clear it is NOT ok to do something like this on master or other branches where you work together with others.

kBashar · 2014-03-08T14:44:09Z

Thanks for comforting.
Okay then I'll experiment with rebase and will modify according to necessity though it may take a day or two as I'm pretty busy with my college stuff.

ywwg · 2014-03-08T15:09:47Z

Since this is a new feature, I'd prefer to hold off merging this until post-1.12.

daschuer · 2014-03-08T19:53:35Z

src/library/searchquery.cpp

@@ -142,6 +142,12 @@ QString TextFilterNode::toSql() const {
        m_operator = operatorMatcher.cap(1);
        argument = operatorMatcher.cap(2);
    }
+	// first check if the query is about duration


Replace this tab with four spaces

daschuer · 2014-03-08T20:17:00Z

Hi @kBashar, thank you for your work!
The code works fine. I have only added some ideas for improvement inline.

ywwg · 2014-03-08T23:09:56Z

Should we create an "experimental" branch for "features we don't want to merge into trunk yet?" Usually I'd suggest we branch 1.12 and merge a patch like this into trunk, but RJ had some reason for not cutting a release branch. The alternative is the contributor keeps their branch up to date with trunk, but that's not really reasonable to expect (and it doesn't happen).

kain88-de · 2014-03-08T23:14:14Z

or we just mark this as a milestone for 1.13. Then we after the release we can filter all PR's and merge the ones marked for 1.13

daschuer · 2014-03-09T10:55:18Z

I like the Idea to have a separate branch. Since we are on git, it is no deal to make this to "master" once the time has come. "Experimental" sounds to unstable to me, what about "post_1.12"

The most easy solution is to treat this just like a bugfix and merge it to master once it is finished

It actually fix the bug "duration:>3:30" did not work ;-)
It does not brake current interface
No risk to break anything outside the scope
minimum work for us
maximum benefit for 1.12 users

esbrandt · 2014-03-10T05:59:16Z

The most easy solution is to treat this just like a bugfix and merge it to master once it is finished

This. The PR fixes an existing feature.

ywwg · 2014-03-10T13:35:51Z

what's the bug number this is fixing?

kBashar · 2014-03-10T14:42:18Z

It's in wishlist and the Bug number is #1261493 (Support human-readable time suffixes in time-based search query filters)

ywwg · 2014-03-10T15:01:10Z

It's called wishlist and it's a feature request, so it's not a bugfix. If we call every feature a bugfix then feature freeze has no meaning. I appreciate the tests, and the relatively small scope, but this type of feature creep has really hurt us in the past.

kain88-de · 2014-03-12T23:02:34Z

I also agree that this is a wishlist bug and should go into 1.13

@kBashar the code seems fine if you address the style issues. Also can you add checks in the tests for the old way of just typing the seconds.

Addresses this (https://bugs.launchpad.net/mixxx/+bug/1261493) wishlist bug MIXXX can only search duration in seconds. Searching tracks with duration keyword is made more natural. User can search now in following formats 2:30/2:30m 2:30:45/2:30:45h 2m30s/2m30 2h30m45s/2h30m45 In user manual we should encourage our users to use top two formats.

kBashar · 2014-03-17T11:30:39Z

@kain88-de I've updated this PR. sorry for late, busy with college schedule.

daschuer · 2014-04-13T22:55:27Z

@ywwg: Do you have still objection to merge this? I am afraid it will start to rod at some point.

ywwg · 2014-04-14T01:35:19Z

src/library/searchquery.cpp

@@ -164,6 +170,122 @@ NumericFilterNode::NumericFilterNode(const QStringList& sqlColumns,

 }

+QString NumericFilterNode::parseHumanReadableTime(QString durationHumanReadable) {
+    QStringList durationRanges = durationHumanReadable.split("-");
+    qDebug()<<durationHumanReadable;


remove this debug

ywwg · 2014-04-14T01:39:55Z

src/library/searchquery.cpp

+        if (inputDuration.endsWith("h",Qt::CaseInsensitive)) {
+               inputDuration.chop(1);
+            }
+        QStringList timeSeparated = inputDuration.split(colone_regexp);


just inline the colon_regexp.

ywwg · 2014-04-14T01:40:39Z

I agree our feature freeze has kind of exploded :). But it looks like this PR needs a little more work before we can merge it.

kBashar · 2014-04-14T16:37:51Z

@ywwg sorry for not being perfect. thanks for your suggestions. I'll check and fix these within tomorrow.

kBashar · 2014-04-17T15:34:14Z

@ywwg I've updated. would you please give it a check?

ywwg · 2014-04-24T00:30:46Z

src/library/searchquery.cpp

@@ -142,6 +142,12 @@ NumericFilterNode::NumericFilterNode(const QStringList& sqlColumns,
        m_operator = operatorMatcher.cap(1);
        argument = operatorMatcher.cap(2);
    }
+    // first check if the query is about duration


I think all search code is supposed to go in searchqueryparser.cpp

ywwg · 2014-04-24T00:35:14Z

Sorry for the delay. I've made more notes. I know that some of them are pretty major, but there's lots of cool stuff to learn along the way :)

kain88-de · 2014-08-04T07:49:09Z

@kBashar are you still working on this?

kain88-de · 2014-08-29T10:37:01Z

Reworked this in #327

GitHub Actions: Add check if build is warning free

kain88-de reviewed Mar 8, 2014
View reviewed changes

kain88-de self-assigned this Mar 8, 2014

daschuer reviewed Mar 8, 2014
View reviewed changes

ywwg reviewed Apr 14, 2014
View reviewed changes

Humanreadable duration search enabled

15da73e

ywwg reviewed Apr 24, 2014
View reviewed changes

kain88-de mentioned this pull request Aug 29, 2014

Humanreadable search #327

Merged

kain88-de closed this Aug 29, 2014

mixxxbot mentioned this pull request Aug 22, 2022

Support human-readable time suffixes in time-based search query filters #7233

Closed

m0dB pushed a commit to m0dB/mixxx that referenced this pull request Jan 21, 2024

Merge pull request mixxxdj#186 from Holzhaus/build-warnings

a74bcf0

GitHub Actions: Add check if build is warning free

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Humanreadable duration search enabled #186

Humanreadable duration search enabled #186

kBashar commented Mar 8, 2014

kain88-de Mar 8, 2014

kain88-de commented Mar 8, 2014

kBashar commented Mar 8, 2014

kain88-de commented Mar 8, 2014

kBashar commented Mar 8, 2014

ywwg commented Mar 8, 2014

daschuer Mar 8, 2014

daschuer commented Mar 8, 2014

ywwg commented Mar 8, 2014

kain88-de commented Mar 8, 2014

daschuer commented Mar 9, 2014

esbrandt commented Mar 10, 2014

ywwg commented Mar 10, 2014

kBashar commented Mar 10, 2014

ywwg commented Mar 10, 2014

kain88-de commented Mar 12, 2014

kBashar commented Mar 17, 2014

daschuer commented Apr 13, 2014

ywwg Apr 14, 2014

ywwg Apr 14, 2014

ywwg commented Apr 14, 2014

kBashar commented Apr 14, 2014

kBashar commented Apr 17, 2014

ywwg Apr 24, 2014

ywwg commented Apr 24, 2014

kain88-de commented Aug 4, 2014

kain88-de commented Aug 29, 2014

Humanreadable duration search enabled #186

Humanreadable duration search enabled #186

Conversation

kBashar commented Mar 8, 2014

kain88-de Mar 8, 2014

Choose a reason for hiding this comment

kain88-de commented Mar 8, 2014

kBashar commented Mar 8, 2014

kain88-de commented Mar 8, 2014

kBashar commented Mar 8, 2014

ywwg commented Mar 8, 2014

daschuer Mar 8, 2014

Choose a reason for hiding this comment

daschuer commented Mar 8, 2014

ywwg commented Mar 8, 2014

kain88-de commented Mar 8, 2014

daschuer commented Mar 9, 2014

esbrandt commented Mar 10, 2014

ywwg commented Mar 10, 2014

kBashar commented Mar 10, 2014

ywwg commented Mar 10, 2014

kain88-de commented Mar 12, 2014

kBashar commented Mar 17, 2014

daschuer commented Apr 13, 2014

ywwg Apr 14, 2014

Choose a reason for hiding this comment

ywwg Apr 14, 2014

Choose a reason for hiding this comment

ywwg commented Apr 14, 2014

kBashar commented Apr 14, 2014

kBashar commented Apr 17, 2014

ywwg Apr 24, 2014

Choose a reason for hiding this comment

ywwg commented Apr 24, 2014

kain88-de commented Aug 4, 2014

kain88-de commented Aug 29, 2014