Problem with Metadata smart_update

[davidfor]

@Kovid:

I have had a problem with the collections displayed in the device list for some time. Occasionally, they wouldn't show. Unplugging the device and plugging it back in would fix it. Because of that, I haven't been bothered about it. But, I've had a couple of reports recently about problems with collections, including a report that when Metadata management was set to automatic, the collections weren't showing. So, I've had a better look at it.

I found a problem in the KoboTouch driver that caused of the problems when the collection management was turned off or there were no collection columns. After fixing that, there were still issues.

The problem is most obvious when there is no metadata.calibre file on the device. So, the second time the device is connected, everything is OK.

The problem is that something is clearing the "device_collections" attribute between the "Get list of books on device" and "Send metadata to device" jobs. I tracked it down to where the metadata from the device is merged with that from the library. This is in gui2/device.py in method "update_book". That calls ebooks/metadata/books/base.py, Metadata:smartupdate. This is called with "replace_metadata=True".

The problem is that smartupdate with that options sets some attributes to None if they are not in the source object. The list of attributes to be treated this way includes "device_collections". As the source object is coming from the database, "device_collections" won't be in the source object and hence will be set to None.

The fix I have is to add "device_collections" to the list SC_FIELDS_COPY_NOT_NULL in ebooks/metadata/book/__init__.py. With that, all the cases I have tested, the collections are displayed correctly.

Searching through the code, I don't think that this will cause a problem. I do not have any non-Kobo devices to test with, so, I can't prove it that way. And, of course, I might have missed something in the code.

[kovidgoyal]

I'll take a look.

[kovidgoyal]

Doesn't putting it in there mean that it will no longer be copied at all by smart_update(). That is likely to have rather far reaching consequences. A more localized fix would probably be to change the update() book method to preserve the contents of that field instead. Something like

dc = getattr(book, 'device_collections', None)
book.smart_update()
if not getattr(book, 'device_collections', None):
    book.device_collections = dc

[davidfor]

That should work, but it feels clumsy.

The code in smart_update that does the actual copy is:

        if replace_metadata:
            # SPECIAL_FIELDS = frozenset(['lpath', 'size', 'comments', 'thumbnail'])
            for attr in SC_COPYABLE_FIELDS:
                setattr(self, attr, getattr(other, attr, 1.0 if
                        attr == 'series_index' else None))
            self.tags = other.tags
            self.cover_data = getattr(other, 'cover_data',
                                      NULL_VALUES['cover_data'])
            self.set_all_user_metadata(other.get_all_user_metadata(make_copy=True))
            for x in SC_FIELDS_COPY_NOT_NULL:
                copy_not_none(self, other, x)
            if callable(getattr(other, 'get_identifiers', None)):
                self.set_identifiers(other.get_identifiers())
            # language is handled below

At the moment, "device_collections" ends up being part of SC_COPYABLE_FIELDS. By putting it in "SC_FIELDS_COPY_NOT_NULL", it gets removed from SC_COPYABLE_FIELDS, bu will still be copied if it has a value.

The current value of SC_FIELDS_COPY_NOT_NULL is:

SC_FIELDS_COPY_NOT_NULL =  frozenset(['lpath', 'size', 'comments', 'thumbnail'])

I think that "device_collections" should be treated the same as "lpath". I can't find anywhere that device_collections is not set by a driver. And the current behaviour means that the device_collections is wiped after matching the books on the device to that in the library.

[kovidgoyal]

Ah ok, makes sense. I was mis-remembering what smart_update did.

Go ahead and send a PR, I'll merge it.