Repository: beetbox/beets

Issue:
Album flexible attributes modifications are not inherited to tracks
As discussed in https://github.com/beetbox/beets/discussions/4742, changing flexible attributes via a `beet modify -a` command does not inherit the changes to the tracks of the album. 

Changing a fixed attribute on an album _does_ inherit the changes to the album-tracks though.

### Problem

Running

```sh
$ beet mod -a flex_attr=new_value search term
```

Does not change `flex_attr` on the album`s tracks,

while running

```sh
$ beet mod -a genre=new_value search term  # genre is a fixed attribute available to albums an album-tracks
```

_does_ change `genre` on the album's tracks.


### Proposed solution

- Streamline the behaviour of `beet modify -a` - Fixed attributes changes as well as flexible attributes changes should be inherited to album-tracks.
- Add a CLI option that prevents inheritance, allowing changing fixed/flex attributes on an album **only**









Discussion:
JOJ0: Hi @sampsyo we talked about this a couple of months ago and agreed that it would make sense to have the same behaviour of inheritance of album-mods no matter if fixed or flex attr https://github.com/beetbox/beets/discussions/4742#discussioncomment-5565092

Now before I add documentation and changelog to this PR I would like to discuss a usability thing: I just added a capital option of `-I` to `beets modify`. Would it make sense to add a non-capital option `-i` that states `Inherit album-changes to tracks (default)" in its help text? One one hand it would make it clear to the user immediately what the default behaviour is, on the other `-i` would be kind of never to be used, since it's the default anyway and it's not configurable. Am I making sense? I just want to make sure we don't overengineer this feature and make it as logical as possible. Certainly docs of `modify` will mention what the default behaviour is, and how it can be changed.
sampsyo: Thanks for getting this started!

At a very high level, I think it may make more sense to make the "inheritance" happen when making the change on the model, not when calling `store`. The `store` command is supposed to mean "take these temporary changes and commit to them, storing them to the on-disk database." In general, it is not supposed to change the values of fields; it is just supposed to *store* the changes that have already been made. Does that make any sense?

This could happen either in `__setitem__`, which we already override for `Item` although not `Album` yet:
https://github.com/beetbox/beets/blob/0c3f428a601cb40c5fd463791df6229d51b0635e/beets/library.py#L633

Or in the `modify` command, if we think this is a `modify`-specific behavior we want.
wisp3rwind: > At a very high level, I think it may make more sense to make the "inheritance" happen when making the change on the model, not when calling `store`. The `store` command is supposed to mean "take these temporary changes and commit to them, storing them to the on-disk database." In general, it is not supposed to change the values of fields; it is just supposed to _store_ the changes that have already been made. Does that make any sense?

My initial reaction was to agree with you, but after a brief glance at the code, I'm not so sure anymore: `Album.store` already has the `track_updates`/`track_deletes` mechanism. Even if one might design this differently now, it would also be unfortunate to introduce a second code path that propagates tags to items. Also, if I'm not mistaken, `Album`s don't keep their tracks around, but `Album.items()` fetches them freshly from the DB. I doubt we would want to do that in each `__setitem__` call, but once per `store` seems reasonable (and, is already being done for `track_updates`).
sampsyo: Hmm; you're right about that—I hadn't thought that through completely (that `store` is where we currently "propagate" changes as it stands). Which means that you can think of synchronizing the album and item metadata as part of the "sync" that `store` represents. Thanks for catching this!
JOJ0: Well, thanks to both of you for clarifying. I have to admit that @sampsyo's first suggestion confused me a little, but no worries, I'm glad we seem to be back on track :-)

Good, before I move on to wrapping my head around all the tests that are failing now, any other suggestions if there is a better way than passing a kwarg through a few levels to make this "configurable"?

Also any opinions on whether or not it makes sense to add an additional `-i` option. I would suggest to not do that. A single option `-I` (capital since we often see that in beets for negation) I'd consider sufficient.
arsaboo: @JOJ0 I thought `-I` is used for non-incremental or something. This is my go-to import command `beet import -m -I -t ~/shared/music/`
sampsyo: > any other suggestions if there is a better way than passing a kwarg through a few levels to make this "configurable"?

The main way would be to make this a proper beets configuration option (i.e., make it appear in `config_default.yaml`). These can be overridden by command-line flags.
JOJ0: > @JOJ0 I thought `-I` is used for non-incremental or something. This is my go-to import command `beet import -m -I -t ~/shared/music/`

Hmm, yes that's true but actually I thought that its not problem to have the same letter for a different subcommand. ‘beet modify -I’ is not ‘beet import -I‘

But yeah if you guys can think of a different letter I'm up for suggestions.

‘beet mod -n‘ better? (but -n often is somethin like --no-op in a lot of cli tools, didnt like it because of that)


arsaboo: If it does not conflict, I am fine with `beet modify -I`
JOJ0: > > any other suggestions if there is a better way than passing a kwarg through a few levels to make this "configurable"?
> 
> The main way would be to make this a proper beets configuration option (i.e., make it appear in `config_default.yaml`). These can be overridden by command-line flags.

Sorry for being unclear with my question. I used the word configurable but put it in double quotes because I was trying to say something like: ....any better way...to implement this?

Well, you did kind of answer to that as well. A regular beets config option that a cli option overrides is probably a typicial form of implementing this. Now my initial idea was to have a config option but after thinking this through a couple of times I came to the conclusion that actually I  consider it really necessary. If I want to modify something, then a consistent default should be used - which we finally achieve with these changes. Finally album mods propagate to items for flex attrs as well (not touching the current behaviour for fixed attrs). If this is not what the user wants, they can use this new cli option to modify.

That werer my thoughts, also trying to keep this change minimal. We can certainly discuss it. Maybe having an option does not hurt....

Any suggestions where an option like this would "live" in config_default.yaml?

Ideas:

```
modify:
    inherit: yes
```

```
modify_inherit: yes
```

```
inherit_modifications: yes
```

```
album_to_item_mods: yes
```

And now, _if_ we would add such an option it would further be required to have a second cli option that can alter the behaviour.

Suggestion:

If user config option set to `no`, this can be used to alter:

`beet modify -i`

If user config option set to `yes` (or using the default), this can be used to alter:

`beet modify -I`

Thoughts welcome! Thanks!
arsaboo: I prefer:
```yaml
modify:
    inherit: yes
```
I am also thinking that the default behavior should be to inherit attributes. 
JOJ0: Next up is this test:
https://github.com/beetbox/beets/blob/b19b961035da23a7cbe2a049d2e3f19e7ce220d5/test/test_library.py#L1025-L1030

Failing like so:
```
======================================================================
FAIL: test_albuminfo_change_artist_does_not_change_items (test.test_library.AlbumInfoTest.test_albuminfo_change_artist_does_not_change_items)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jojo/git/beets/test/test_library.py", line 1030, in test_albuminfo_change_artist_does_not_change_items
    self.assertNotEqual(i.artist, 'myNewArtist')
AssertionError: 'myNewArtist' == 'myNewArtist'

----------------------------------------------------------------------
Ran 12 tests in 0.216s

FAILED (failures=1)
```

It seems like inheriting change of artist (of the album) shouldn't be inherited to items which this check proves.

I could easily fix this test by passing `inherit=False` to the `store()` when running the test:
https://github.com/beetbox/beets/pull/4823/commits/9d0a4ddb12a5b80b2f6b2a2d28f0164d3f4bf102

but is this what we want in general: Should editing of artist always not be inherited to items?

On the other hand editing of an albumartist (of an album) is checked whether succesfully propagated to items with this test:
https://github.com/beetbox/beets/blob/b19b961035da23a7cbe2a049d2e3f19e7ce220d5/test/test_library.py#L1017-L1023



sampsyo: > I'm not sure if this is what we want. High level question: I would say the 'id' field should always be prevented to propagate from album mods to items? If yes, there might be other fields that should always not be inherited. I could put a CONSTANT somewhere that collects these fields. Would that be the right way?

Good question. Yes, absolutely; we would never ever want the `id` field to propagate to items; that does indeed seem like a recipe for confusion. But it's a good question to ask whether there are _other_ things that are similar… my gut reaction says "no, `id` stands alone," but I could very well be proven wrong. Maybe we should just watch out for other related effects where this kind of alignment is clearly undesirable?

> I could easily fix this test by passing `inherit=False` to `store()` when running the test: [9d0a4dd](https://github.com/beetbox/beets/commit/9d0a4ddb12a5b80b2f6b2a2d28f0164d3f4bf102)
> 
> but is this what we want in general? Should editing of artist perhaps never propagate to items?

It seems like this test was expressly meant to "lock in" the old behavior, which we are now changing. I think what seems reasonable here is that we should split this into two tests: one with `inherit=True` and one with `inherit=False`, and we check that the resulting behavior differs in the expected way between the two.
JOJ0: Ready for a final review @sampsyo, maybe have a look at the docs I wrote, I also reworded existing stuff for `mod -a` a little. Is that easily understandable and correct like that: https://github.com/beetbox/beets/pull/4823/commits/d30ac6032cbdc317a479ec760acef3e0da411c52

I left the check for ìd`simply inline with the if-statement. An extra GLOBAL_NEVER_INHERIT_THIS_FIELD or something like that felt overkill: https://github.com/beetbox/beets/pull/4823/commits/7313fe1d5f3806e2612ee45b3578f9ed7068da64

As a final step I tried to clarify and extend docstring and comments within this function. Nitpicking appreciated since it is a very very core-thing everybody who comes accross it should have the chance to understand quickly. What do you say? Does that make sense? https://github.com/beetbox/beets/pull/4823/commits/a6937f08f293f04a32a127204855ef643afd1e1d



Patch:
diff --git a/beets/library.py b/beets/library.py
index 46d2f416c..405b546e2 100644
--- a/beets/library.py
+++ b/beets/library.py
@@ -1369,22 +1369,27 @@ class Album(LibModel):
 
         plugins.send('art_set', album=self)
 
-    def store(self, fields=None):
+    def store(self, fields=None, inherit=True):
         """Update the database with the album information.
 
-        The album's tracks are also updated.
-
         `fields` represents the fields to be stored. If not specified,
         all fields will be.
+
+        The album's tracks are also updated when the `inherit` flag is enabled.
+        This applies to fixed attributes as well as flexible ones. The `id`
+        attribute of the album will never be inherited.
         """
         # Get modified track fields.
         track_updates = {}
         track_deletes = set()
         for key in self._dirty:
-            if key in self.item_keys:
-                track_updates[key] = self[key]
-            elif key not in self:
-                track_deletes.add(key)
+            if inherit:
+                if key in self.item_keys:  # is a fixed attribute
+                    track_updates[key] = self[key]
+                elif key not in self:  # is a fixed or a flexible attribute
+                    track_deletes.add(key)
+                elif key != 'id':  # is a flexible attribute
+                    track_updates[key] = self[key]
 
         with self._db.transaction():
             super().store(fields)
@@ -1400,7 +1405,7 @@ class Album(LibModel):
                             del item[key]
                     item.store()
 
-    def try_sync(self, write, move):
+    def try_sync(self, write, move, inherit=True):
         """Synchronize the album and its items with the database.
         Optionally, also write any new tags into the files and update
         their paths.
@@ -1409,7 +1414,7 @@ class Album(LibModel):
         `move` controls whether files (both audio and album art) are
         moved.
         """
-        self.store()
+        self.store(inherit=inherit)
         for item in self.items():
             item.try_sync(write, move)
 
diff --git a/beets/ui/commands.py b/beets/ui/commands.py
index 750457234..636d9b8f4 100755
--- a/beets/ui/commands.py
+++ b/beets/ui/commands.py
@@ -1489,7 +1489,7 @@ default_commands.append(version_cmd)
 
 # modify: Declaratively change metadata.
 
-def modify_items(lib, mods, dels, query, write, move, album, confirm):
+def modify_items(lib, mods, dels, query, write, move, album, confirm, inherit):
     """Modifies matching items according to user-specified assignments and
     deletions.
 
@@ -1542,7 +1542,7 @@ def modify_items(lib, mods, dels, query, write, move, album, confirm):
     # Apply changes to database and files
     with lib.transaction():
         for obj in changed:
-            obj.try_sync(write, move)
+            obj.try_sync(write, move, inherit)
 
 
 def print_and_modify(obj, mods, dels):
@@ -1585,7 +1585,8 @@ def modify_func(lib, opts, args):
     if not mods and not dels:
         raise ui.UserError('no modifications specified')
     modify_items(lib, mods, dels, query, ui.should_write(opts.write),
-                 ui.should_move(opts.move), opts.album, not opts.yes)
+                 ui.should_move(opts.move), opts.album, not opts.yes,
+                 opts.inherit)
 
 
 modify_cmd = ui.Subcommand(
@@ -1613,6 +1614,10 @@ modify_cmd.parser.add_option(
     '-y', '--yes', action='store_true',
     help='skip confirmation'
 )
+modify_cmd.parser.add_option(
+    '-I', '--noinherit', action='store_false', dest='inherit', default=True,
+    help="when modifying albums, don't also change item data"
+)
 modify_cmd.func = modify_func
 default_commands.append(modify_cmd)
 
diff --git a/beetsplug/ipfs.py b/beetsplug/ipfs.py
index 5794143bd..11f131418 100644
--- a/beetsplug/ipfs.py
+++ b/beetsplug/ipfs.py
@@ -296,4 +296,4 @@ class IPFSPlugin(BeetsPlugin):
         self._log.info("Adding '{0}' to temporary library", album)
         new_album = tmplib.add_album(items)
         new_album.ipfs = album.ipfs
-        new_album.store()
+        new_album.store(inherit=False)
diff --git a/docs/changelog.rst b/docs/changelog.rst
index ee8f4c2f6..b539c6d99 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -211,6 +211,11 @@ Bug fixes:
   the highest number of likes
 * :doc:`/plugins/lyrics`: Fix a crash with the Google backend when processing
   some web pages. :bug:`4875`
+* Modifying flexible attributes of albums now cascade to the individual album
+  tracks, similar to how fixed album attributes have been cascading to tracks
+  already. A new option ``--noinherit/-I`` to :ref:`modify <modify-cmd>`
+  allows changing this behaviour.
+  :bug:`4822`
 
 For packagers:
 
diff --git a/docs/reference/cli.rst b/docs/reference/cli.rst
index da119d0f8..9306397a9 100644
--- a/docs/reference/cli.rst
+++ b/docs/reference/cli.rst
@@ -257,7 +257,7 @@ modify
 ``````
 ::
 
-    beet modify [-MWay] [-f FORMAT] QUERY [FIELD=VALUE...] [FIELD!...]
+    beet modify [-IMWay] [-f FORMAT] QUERY [FIELD=VALUE...] [FIELD!...]
 
 Change the metadata for items or albums in the database.
 
@@ -274,13 +274,17 @@ name into the artist field for all your tracks,
 and ``beet modify title='$track $title'`` will add track numbers to their
 title metadata.
 
-The ``-a`` switch also operates on albums in addition to the individual tracks.
+The ``-a`` option changes to querying album fields instead of track fields and
+also enables to operate on albums in addition to the individual tracks.
 Without this flag, the command will only change *track-level* data, even if all
 the tracks belong to the same album. If you want to change an *album-level*
 field, such as ``year`` or ``albumartist``, you'll want to use the ``-a`` flag
 to avoid a confusing situation where the data for individual tracks conflicts
 with the data for the whole album.
 
+Modifications issued using ``-a`` by default cascade to individual tracks. To
+prevent this behavior, use ``-I``/``--noinherit``.
+
 Items will automatically be moved around when necessary if they're in your
 library directory, but you can disable that with  ``-M``. Tags will be written
 to the files according to the settings you have for imports, but these can be


Test patch:
diff --git a/test/test_ipfs.py b/test/test_ipfs.py
index 8f72f5132..593a01b8f 100644
--- a/test/test_ipfs.py
+++ b/test/test_ipfs.py
@@ -87,7 +87,7 @@ class IPFSPluginTest(unittest.TestCase, TestHelper):
 
         album = self.lib.add_album(items)
         album.ipfs = "QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf"
-        album.store()
+        album.store(inherit=False)
 
         return album
 
diff --git a/test/test_library.py b/test/test_library.py
index 0e9637635..269771575 100644
--- a/test/test_library.py
+++ b/test/test_library.py
@@ -1022,10 +1022,17 @@ class AlbumInfoTest(_common.TestCase):
         self.assertEqual(i.albumartist, 'myNewArtist')
         self.assertNotEqual(i.artist, 'myNewArtist')
 
+    def test_albuminfo_change_artist_does_change_items(self):
+        ai = self.lib.get_album(self.i)
+        ai.artist = 'myNewArtist'
+        ai.store(inherit=True)
+        i = self.lib.items()[0]
+        self.assertEqual(i.artist, 'myNewArtist')
+
     def test_albuminfo_change_artist_does_not_change_items(self):
         ai = self.lib.get_album(self.i)
         ai.artist = 'myNewArtist'
-        ai.store()
+        ai.store(inherit=False)
         i = self.lib.items()[0]
         self.assertNotEqual(i.artist, 'myNewArtist')