[PATCH 1/3] tree: do not use artistsort for compilations

[Johannes Weißl]

Otherwise non-VA compilations get split into multiple tree entries if
tracks have set artistsort.
---
comment.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/comment.c b/comment.c
index eab35e3..da2a801 100644
--- a/comment.c
+++ b/comment.c
@@ -60,9 +60,13 @@ const char *comments_get_artistsort(const struct keyval *comments)
return NULL;

val = keyvals_get_val(comments, "albumartistsort");
+ if (!track_is_compilation(comments)) {
+ if (!val || strcmp(val, "") == 0)
+ val = keyvals_get_val(comments, "artistsort");
+ }

if (!val || strcmp(val, "") == 0)
- val = keyvals_get_val(comments, "artistsort");
+ return NULL;

return val;
}

[Johannes Weißl]

Toggling smart_artist_sort can lead to artists collapsing / splitting,
adapt tree_sort_artists() to handle that. E.g.:

track1: name="The Beatles", sortname="Beatles, The"
track2: name="The Beatles"

Also adding tracks can lead to name changes, e.g.:

track1: name="Beatles, The"
track2: name="The Beatles"

(name of track1 should change to name of track2)
---
tree.c | 55 +++++++++++++++++++++++++++++++++++++++----------------
1 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/tree.c b/tree.c
index d81472d..7662fbf 100644
--- a/tree.c
+++ b/tree.c
@@ -568,21 +568,6 @@ static void insert_artist(struct artist *artist, struct rb_root *root)
}
}

-void tree_sort_artists(void)
-{
- struct rb_node *node, *tmp;
- struct rb_root tmptree = RB_ROOT;
-
- rb_for_each_safe(node, tmp, &lib_artist_root) {
- struct artist *a = to_artist(node);
- rb_erase(node, &lib_artist_root);
- insert_artist(a, &tmptree);
- }
-
- lib_artist_root.rb_node = tmptree.rb_node;
- window_changed(lib_tree_win);
-}
-
static void add_artist(struct artist *artist)
{
insert_artist(artist, &lib_artist_root);
@@ -747,11 +732,27 @@ void tree_add_track(struct tree_track *track)
new_album = album_new(new_artist, album_name, date, is_va_compilation);

if (artist) {
+ int changed = 0;
/* If it makes sense to update sort_name, do it */
if (!artist->sort_name && artistsort_name) {
artist->sort_name = xstrdup(artistsort_name);
artist->collkey_sort_name = u_strcasecoll_key(artistsort_name);
-
+ changed = 1;
+ }
+ /* If names differ, update */
+ if (!artist->auto_sort_name) {
+ char *auto_sort_name = auto_artist_sort_name(artist_name);
+ if (auto_sort_name) {
+ free(artist->name);
+ free(artist->collkey_name);
+ artist->name = xstrdup(artist_name);
+ artist->collkey_name = u_strcasecoll_key(artist_name);
+ artist->auto_sort_name = auto_sort_name;
+ artist->collkey_auto_sort_name = u_strcasecoll_key(auto_sort_name);
+ changed = 1;
+ }
+ }
+ if (changed) {
remove_artist(artist);
add_artist(artist);
window_changed(lib_tree_win);
@@ -975,6 +976,28 @@ void tree_remove_sel(void)
}
}

+void tree_sort_artists(void)
+{
+ struct rb_node *a_node, *a_tmp;
+
+ rb_for_each_safe(a_node, a_tmp, &lib_artist_root) {
+ struct rb_node *l_node, *l_tmp;
+ struct artist *artist = to_artist(a_node);
+
+ rb_for_each_safe(l_node, l_tmp, &artist->album_root) {
+ struct rb_node *t_node, *t_tmp;
+ struct album *album = to_album(l_node);
+
+ rb_for_each_safe(t_node, t_tmp, &album->track_root) {
+ struct tree_track *track = to_tree_track(t_node);
+
+ tree_remove(track);
+ tree_add_track(track);
+ }
+ }
+ }
+}
+
void tree_sel_current(void)
{
tree_sel_track(lib_cur_track);

[Johannes Weißl]

* Use linear search to find albums. This is necessary to find albums
where the date differs. There is no measurable slowdown because of
this.
* Decide the sorting (compilation: name, regular: date) based on the
artist. Otherwise special_album_cmp() is no strict weak ordering
required for an rbtree (thanks to Gregory for pointing that out!).

This fixes:
* Same album split into several in case tracks in it have different
dates.
* Possible memory corruption of the tree.
---
lib.h | 3 +--
tree.c | 49 +++++++++++++++++++++++++++++++++----------------
2 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/lib.h b/lib.h
index 1a6d38b..e882497 100644
--- a/lib.h
+++ b/lib.h
@@ -45,8 +45,6 @@ struct album {
char *collkey_name;
/* date of the first track added to this album */
int date;
-
- int is_compilation;
};

struct artist {
@@ -65,6 +63,7 @@ struct artist {

/* albums visible for this artist in the tree_win? */
unsigned int expanded : 1;
+ unsigned int is_compilation : 1;
};

const char *artist_sort_name(const struct artist *);
diff --git a/tree.c b/tree.c
index 7662fbf..f40d667 100644
--- a/tree.c
+++ b/tree.c
@@ -377,7 +377,7 @@ static char *auto_artist_sort_name(const char *name)
return buf;
}

-static struct artist *artist_new(const char *name, const char *sort_name)
+static struct artist *artist_new(const char *name, const char *sort_name, int is_compilation)
{
struct artist *a = xnew(struct artist, 1);

@@ -388,6 +388,7 @@ static struct artist *artist_new(const char *name, const char *sort_name)
a->collkey_sort_name = u_strcasecoll_key0(a->sort_name);
a->collkey_auto_sort_name = u_strcasecoll_key0(a->auto_sort_name);
a->expanded = 0;
+ a->is_compilation = is_compilation;
rb_root_init(&a->album_root);

return a;
@@ -404,7 +405,7 @@ static void artist_free(struct artist *artist)
free(artist);
}

-static struct album *album_new(struct artist *artist, const char *name, int date, int is_compilation)
+static struct album *album_new(struct artist *artist, const char *name, int date)
{
struct album *album = xnew(struct album, 1);

@@ -413,7 +414,6 @@ static struct album *album_new(struct artist *artist, const char *name, int date
album->date = date;
rb_root_init(&album->track_root);
album->artist = artist;
- album->is_compilation = is_compilation;

return album;
}
@@ -498,18 +498,19 @@ static int special_name_cmp(const char *a, const char *collkey_a,

static int special_album_cmp(const struct album *a, const struct album *b)
{
+ return special_name_cmp(a->name, a->collkey_name, b->name, b->collkey_name);
+}
+
+static int special_album_cmp_date(const struct album *a, const struct album *b)
+{
/* keep <Stream> etc. top */
int cmp = (*a->name != '<') - (*b->name != '<');
-
if (cmp)
return cmp;

- /*
- * Sort regular albums by date, but sort compilations
- * alphabetically.
- */
- if (a->date != b->date && !a->is_compilation && !b->is_compilation)
- return a->date - b->date;
+ cmp = a->date - b->date;
+ if (cmp)
+ return cmp;

return strcmp(a->collkey_name, b->collkey_name);
}
@@ -579,6 +580,7 @@ static struct artist *find_artist(const struct artist *artist)
}

static struct album *do_find_album(const struct album *album,
+ int (*cmp)(const struct album *, const struct album *),
struct rb_node ***p_new,
struct rb_node **p_parent)
{
@@ -587,7 +589,7 @@ static struct album *do_find_album(const struct album *album,
while (*new) {
struct album *a = to_album(*new);

- int result = special_album_cmp(album, a);
+ int result = cmp(album, a);

parent = *new;
if (result < 0)
@@ -606,7 +608,15 @@ static struct album *do_find_album(const struct album *album,

static struct album *find_album(const struct album *album)
{
- return do_find_album(album, NULL, NULL);
+ struct album *a;
+ struct rb_node *tmp;
+
+ /* do a linear search because we want find albums with different date */
+ rb_for_each_entry(a, tmp, &album->artist->album_root, tree_node) {
+ if (special_album_cmp(album, a) == 0)
+ return a;
+ }
+ return NULL;
}

static void add_album(struct album *album)
@@ -614,7 +624,14 @@ static void add_album(struct album *album)
struct rb_node **new = &(album->artist->album_root.rb_node), *parent = NULL;
struct album *found;

- found = do_find_album(album, &new, &parent);
+ /*
+ * Sort regular albums by date, but sort compilations
+ * alphabetically.
+ */
+ found = do_find_album(album,
+ album->artist->is_compilation ? special_album_cmp
+ : special_album_cmp_date,
+ &new, &parent);
if (!found) {
rb_link_node(&album->tree_node, parent, new);
rb_insert_color(&album->tree_node, &album->artist->album_root);
@@ -718,18 +735,18 @@ void tree_add_track(struct tree_track *track)
is_va_compilation = ti->is_va_compilation;
}

- new_artist = artist_new(artist_name, artistsort_name);
+ new_artist = artist_new(artist_name, artistsort_name, is_va_compilation);
new_album = album = NULL;

artist = find_artist(new_artist);
if (artist) {
artist_free(new_artist);
- new_album = album_new(artist, album_name, date, is_va_compilation);
+ new_album = album_new(artist, album_name, date);
album = find_album(new_album);
if (album)
album_free(new_album);
} else
- new_album = album_new(new_artist, album_name, date, is_va_compilation);
+ new_album = album_new(new_artist, album_name, date);

if (artist) {
int changed = 0;

[Gregory Petrosyan]

Finally got time to review it -- merged from pu to master, thanks!

Gregory