caleb531 / play-song

An Alfred workflow for quickly and easily playing music in the Apple Music app
MIT License
107 stars 9 forks source link

Better sorting of results #11

Closed tyilo closed 9 years ago

tyilo commented 9 years ago

Results should not be sorted alphabetically, but by the relevance.

Example:

I would expect "OK Computer" to come before "Bookends", because "ok" is in the middle of "Bookends" and is in the beginning of "OK Computer".

I suggest sorting like so (assuming that there are no spaces in the query):

  1. Output all results beginning with the query
  2. Output all results having a word that begins with the query
  3. Output all results containing the query
  4. Output all remaining results
caleb531 commented 9 years ago

Sorting the results in this way would introduce a significant performance penalty, especially for libraries of increasing size. Thankfully, Alfred utilizes the unique ID of each result to keep track of which results have been most frequently/recently chosen. Alfred then moves results with the highest "frecency" to the top of the list. Therefore, as long as you are specific at first (to find the result you want), Alfred should consider that result more relevant on successive searches.

tyilo commented 9 years ago

Are you sure about the performance penalty? I would believe the bottleneck is getting the info out of iTunes.

caleb531 commented 9 years ago

Yes, retrieving data from iTunes is the main bottleneck, but also because AppleScript isn't very expressive relative to other programming languages. Thus, common actions (such as sorting) are a bit more cumbersome to implement (and therefore more inefficient because of the manual work required).

Caleb

tyilo commented 9 years ago

I'll try implementing this for one of the filters and see if you find it too slow.

caleb531 commented 9 years ago

Be my guest. I welcome any creative solutions to this issue. :)

Caleb

tyilo commented 9 years ago

Take a look at my fork of play-song and try the play2 keyword instead of play. Currently it is a little bit slower, but I could probably optimize it a lot.

Example result:

caleb531 commented 9 years ago

Ah, of course! It didn't occur to me that I could retrieve the results once, then utilize multiple lists to group the results accordingly. Also, in testing your forked workflow, I did not notice any particular performance penalty (as far as I could tell). However, I would very much prefer to rewrite the logic for implementing this behavior to be more DRY. Fortunately, I have already thought of a very feasible solution for doing this.

Therefore, I am reopening this issue because I am indeed interested in implementing this behavior. I plan to start on this within the coming days and will push a new branch when I make some solid progress.

Good job. :) Caleb

tyilo commented 9 years ago

Cool.

I agree, all the filter-xxx.applescript files have a lot of duplicated code.

caleb531 commented 9 years ago

Yes, much of the code across those files is very similar (if not the same in some sections). However, there are subtle differences in logic (e.g. get every track whose name... vs. get every track whose artist) which aren't easily abstractable. If AppleScript were a bit more functional (e.g. if I could use a string in place of a keyword such as artist), then it would be dramatically simpler to abstract that logic out into its own reuseable handler. Unfortunately, AppleScript is not designed to be so succinctly expressive.

Anyway, I've finished implementing the logic for result sorting, which you can find on the feature/sorting branch. Please browse the code and test the workflow and let me know what you think. :)

Caleb

tyilo commented 9 years ago

Have you considered using JavaScript for Automation instead of AppleScript? I don't know much about it, but wouldn't that make it possible to make the code more modulized.

Of course that is only supported on Yosemite or later.

caleb531 commented 9 years ago

I haven't heard of JavaScript for Automation before, but after a quick Google search, I am quite intrigued. However, as you mentioned, compatibility with older version of OS X is important to me, and so the prospect of switching to JXA is not worth considering currently. I'd also prefer to wait and see how JXA develops and becomes more widely recognized and adopted. Additionally, for all I know, Alfred currently may have no way of recognizing JXA.

Anyway, you still haven't responded to my implied questions. How do you like my implementation of the sorting logic? Does the workflow work for you?

Caleb

tyilo commented 9 years ago

Doesn't work for me, play lo returns the same result as master branch, not the same as my play2.

Also see the comment on the branch's commit.

caleb531 commented 9 years ago

After further examination of the code I've written, I can see how my implementation of the result categorization fails to produce the desired result. The logic ultimately fails because it only categorizes the first n results (determined by resultLimit) to which the full list of results has been constrained.

In addition, after further testing of your play2 filter, I have actually noticed somewhat of a performance penalty. When I type a single letter (e.g. "play2 L"), the workflow takes considerably more time to return results (roughly 2 1/4 seconds), whereas play L returns the results much more quickly (half a second or less). Therefore, I am not inclined to adopt your categorization implementation, nor am I even inclined to adopt my own.

However, I am not yet ready to yield. I still believe that this behavior is achievable implementation-wise. I simply need to continue my testing of which method of filtering results works best. I'm actually beginning to lean towards having iTunes do the heavy lifting, as it's searching commands seems to be optimized enough to search across the entire library very quickly.

I'll be sure to comment here with any new findings.

Caleb

tyilo commented 9 years ago

A simple solution would be to only use the sort the results if the number of results is lower than some limit, for instance 100.

caleb531 commented 9 years ago

Hmm, that is indeed a possibility. I'll consider that as I continue brainstorming and testing. :)

caleb531 commented 9 years ago

Well, after much thought and experimentation, I have crafted two test cases to demonstrate two approaches which I believe are the most feasible. Fair warning: I have already made my choice regarding which approach I'd prefer to implement (I'll explain why). Both scripts are included below—feel free to try them for yourself.

Approach 1 involves retrieving the tracks without having iTunes filter it much. Consequently, most of the filtering and categorization logic is implemented manually.

-- approach 1
-- tested via osascript
on getSongs(query)

    set songLimit to 9

    tell application "iTunes"

        set theSongs to get every track in playlist 2 whose name contains query

        set songCategories to {{}, {}, {}, {}}
        set songNames to {}

        repeat with theSong in theSongs

            set songName to name of theSong

            if songName begins with query
                set categoryIndex to 1
            else if songName contains (space & query)
                set categoryIndex to 2
            else if songName contains query
                set categoryIndex to 3
            else
                set categoryIndex to 4
            end if

            copy songName to end of item categoryIndex of songCategories

        end repeat

        -- concatenate categories into song names list
        repeat with songCategory in songCategories

            set songNames to songNames & songCategory

        end

        -- constrain list
        if length of songNames > songLimit then

            set songNames to items 1 thru songLimit of songNames

        end if

        return length of songNames

    end tell

end getSongs

getSongs("E")

Approach 2 delegates much of the heavy lifting to iTunes in terms of filtering and ordering. The logic isn't as DRY as I'd like, but it still seems very straightforward, readable, and unambiguous. Running via osascript, this approach is roughly twice as fast as approach 1. Therefore, because performance is a high priority for me, I opt for this approach. Moreover, this approach should actually be much simpler and less messy to implement across all of the script filters compared to approach 1 (because most of the remaining script filter code is untouched).

-- approach 2
-- runs twice as fast compared to approach 1
-- tested via osascript
on getSongs(query)

    set songLimit to 9

    tell application "iTunes"

        -- I realize this isn't as DRY as I'd like it to be, but it
        set theSongs to {}
        set theSongs to theSongs & (get every track in playlist 2 whose name starts with query)
        set theSongs to theSongs & (get every track whose name contains (space & query) and name does not start with query)
        set theSongs to theSongs & (get every track whose name contains query and name does not start with query and name does not contain (space & query))

        -- constrain list
        -- this can be above loop b/c iTunes has already done the heavy lifting
        if length of theSongs > songLimit then

            set theSongs to items 1 thru songLimit of theSongs

        end if

        set songNames to {}

        repeat with theSong in theSongs

            set songNames to songNames & (name of theSong)
            -- no need for additional categorization logic; already done

        end repeat

        return length of songNames

    end tell

end getSongs

getSongs("E")

Very curious to hear your thoughts, @Tyilo. Caleb

tyilo commented 9 years ago

If the second solution is better, then that's fine with me.

I made a DRY version of your second version:

on getResults(query, queryType)

    set resultLimit to 9

    set evalScript to run script "
    script
        on findResults(query, queryType, resultLimit)
            tell application \"iTunes\"
                set theSongs to {}
                set theSongs to theSongs & (get every track in playlist 2 whose " & queryType & " starts with query)
                set theSongs to theSongs & (get every track whose " & queryType & " contains (space & query) and " & queryType & " does not start with query)
                set theSongs to theSongs & (get every track whose " & queryType & " contains query and " & queryType & " does not start with query and " & queryType & " does not contain (space & query))

                if queryType equals \"name\" then
                    if length of theSongs > resultLimit then
                        set theSongs to items 1 thru resultLimit of theSongs
                    end if

                    return theSongs
                else
                    set theResults to {}

                    repeat with theSong in theSongs
                        if length of theResults > resultLimit then
                            exit repeat
                        end if

                        set theResult to " & queryType & " of theSong
                        if theResult is not in theResults then
                            set theResults to theResults & theResult
                        end if
                        end repeat

                    return theResults
                end if

            end tell
        end
    end script
    "

    evalScript's findResults(query, queryType, resultLimit)

end getResults

{item 1 of getResults("E", "name"), item 1 of getResults("E", "album"), item 1 of getResults("E", "genre"), item 1 of getResults("E", "artist")}

It can handle searching for names, albums, genres and artists! You need something specialized for playlist and I don't know the difference between filter-songs and filter-songs-by-name.

Also, could you explain what playlist 2 means? Why is it only in the first get every track query and not in the other 2?

caleb531 commented 9 years ago

I appreciate your effort to make the code more DRY. However, I'm not at all a fan of the eval approach. I'd rather simply integrate the second approach (even if it's not perfectly DRY) and then tweak the code as I find new ways to optimize it.

Now, allow me to address all of your questions:

Firstly, according to the README:

To play an individual song, use the play keyword. Songs which match your query will populate the list of results. Choosing a song from the list will play that song once.

Alternatively, for a more refined search, use the playsong keyword. Songs whose names match your query will populate the list of results. Choosing a song from the list will also play that song once.

In other words, filter-songs-by-name matches by song name only, whereas filter-songs matches by song name, album, artist, etc. (using the awesome search iTunes AS command).

Secondly, playlist 2 refers to the main Music playlist in iTunes. The reason I use playlist 2 as opposed to playlist "Music" was because it works better internationally. I actually made this switch when I once received a bug report from someone in Germany, and their "Music" playlist was actually named "Musik".

Thirdly, get every track in playlist 2 should indeed be written on each of those three lines. I simply forgot to copy it down.

I hope that addresses your questions. Caleb

tyilo commented 9 years ago

Thanks for explaining all that ;)

You could cut down a lot of code by using the eval approach. Even though it may not look ugly, you have to use these kinds of hacks when the AppleScript language lacks support doing it without.

I would prefer that over 4x copy and paste.

caleb531 commented 9 years ago

Hmm, you make a compelling argument, and you have persuaded me successfully. Therefore, I will integrate your eval-based approach into a fresh feature/sorting branch and start again on a new implementation. If all goes well, then this should dramatically simplify the codebase.

Caleb

tyilo commented 9 years ago

Yay ;)

caleb531 commented 9 years ago

Well, I am very pleased to report that I have had incredible success in rewriting the sorting logic. In fact, by implementing your eval approach and refactoring some other components, I have been able to make the code remarkably more DRY and maintainable. And the best part: there is no noticeable performance penalty, as far as I can tell.

Please review and test my changes on feature/sorting, and let me know what you think! :)

Caleb

tyilo commented 9 years ago

I think somehow the sorting should be implemented for play as that is the main keyword. I can see that it might be problematic to implement, but I think it's necessary.

caleb531 commented 9 years ago

Yes, I would very much like to implement it for play also, but as you mentioned, the logic is somewhat more elaborate. I've thinking that if I can somehow combine the resulting lists from multiple getResultsFromQuery calls, then I could achieve the desired list of results. However, I am unsure of how that may affect performance. I welcome any input you have regarding this matter. :)

In addition—if you were wondering—I do intend on implementing the sorting behavior for the playlist filter as well. I also thought that because the logic diverges from the existing solution we agreed upon (retrieving playlists instead of tracks), then I might as well write the retrieval/filtering code inline (since it will only need to be added once). I'll begin work on that in a little bit. ;)

Caleb