Add overlay modes for metadata; overwrite, add missing etc. #598

mizaki · 2024-02-04T20:50:08Z

This is the barebones of some overlay options. It's missing the settings which I will add if you are happy with the general idea. It's also missing any tests for the new options. I have done nothing with the notes as yet.

When it comes to the notes, using CVDB xxxxx will still work with Mylar3 and as long as Issue ID is not used (such as Metron Issue ID and instead Metron ID) then the multi-source Tagged with CT etc. etc. CVDB xxxxx, GCD xxxxxx, Metron xxxxx will not cause problems. I've checked Komga, Kavita and LANraragi and none appear to use the notes field.

comicapi/genericmetadata.py

mizaki · 2024-02-14T00:28:58Z

I've tried to simplify things with ddbc5d8 (just the assign_ changes for now) I didn't know if it's better check if a string is 0 rather that just set the cur to whatever new is even if it was an empty string?

mizaki · 2024-02-14T00:52:19Z

When it comes to saving the setting, is it best to use the Enum number or name?

lordwelch · 2024-02-14T01:21:18Z

When it comes to saving the setting, is it best to use the Enum number or name?

parser.add_setting("value", default=ENUM.VALUE, type=ENUM, cmdline=False)

This won't work with int values with our current file loading code.
We would need or isinstance(default, Enum) or (isinstance(setting.type, type) and issubclass(setting.type, Enum)) added to

comictagger/comictaggerlib/ctsettings/__init__.py

Line 58 in 09490b8

if isinstance(value, str):

mizaki · 2024-02-14T01:38:58Z

Is 75c825b in the right area with the class?

lordwelch

Add a single test for each mode that covers each type so that we know what it actually does, it will also make some of the mistakes a little more obvious.

Comments are in the wrong place now but still apply.

comicapi/genericmetadata.py

mizaki · 2024-02-15T22:03:36Z

I'm trying to understand the current overlay:

    def assign_overlay(self, cur: str, new: Any) -> None:
        """Overlay - When the new object has non-None values, over-write them to this one."""
        if new is not None:
            if isinstance(new, str) and len(new) == 0:
                if isinstance(getattr(self, cur), (list, set)): # If an empty string is passed to a list/set field then it is cleared
                    getattr(self, cur).clear()
                else:
                    setattr(self, cur, None) # Otherwise it's any empty string in a string field and should set to None
            elif isinstance(new, (list, set)) and len(new) == 0: # If an empty list/set is passed, do nothing. Why does this not clear the field the same as an empty string?
                pass
            else:
                setattr(self, cur, new) # No type checking, just set the `cur` to `new`

Should an empty list/set not clear it? Am I missing something?

And in general, should assign be doing type checking?

lordwelch · 2024-02-15T23:32:02Z

An empty list is the default

mizaki · 2024-02-16T01:04:35Z

An empty list is the default

But should a list/set expect an empty string? That should be a type error?

mizaki · 2024-02-16T01:05:25Z

I've added some tests and given each one a doc string so hopefully it makes more sense now. Credit tests are left to do.

lordwelch · 2024-02-16T02:27:56Z

But should a list/set expect an empty string? That should be a type error?

I believe this was set in order to make the -m option work for clearing lists

mizaki · 2024-02-16T17:07:49Z

I had to duplicate the StrEnum code from resulttype.py as using it caused an import loop. I guess the best thing to do is move it to a new/another file, if so, which one?

lordwelch · 2024-02-16T22:00:41Z

I had to duplicate the StrEnum code from resulttype.py as using it caused an import loop. I guess the best thing to do is move it to a new/another file, if so, which one?

Copying it is fine, it'll get removed eventually when CT no longer supports 3.10

comicapi/genericmetadata.py

tests/genericmetadata_test.py

comicapi/genericmetadata.py

tests/genericmetadata_test.py

lordwelch · 2024-02-17T04:17:03Z

Also as a nitpick (because I don't remember to do this all the time anyways) you can change:

        if new is not None:
            ...

to

    if new is None:
        return None
    ...

to remove another indent level.

As it is now it's 10x better than the 8 levels of indent originally.

mizaki · 2024-02-17T22:16:12Z

Part of the problem with overwrite and overwrite_no_clear was me not understanding the issue of a string clearing a list/set.

Currently

comictagger/comictaggerlib/ctsettings/types.py

Line 86 in 09490b8

def parse_metadata_from_string(mdstr: str) -> GenericMetadata:

doesn't do any type conversions which it probably should here?

comictagger/comictaggerlib/ctsettings/types.py

Line 132 in 09490b8

setattr(md, key, value)

Or, the GenericMetadata.__post_init__ could do checks and conversions?
That way, overlay doesn't need to worry about the wrong type when replacing/adding/merging.

mizaki · 2024-03-02T21:27:27Z

I've think I finally see the issue with the -m and clearing list/sets. Using the wrong type for the commandline metadata for a list/set does seem the easiest solution. The other option might be: Filename, commandline, read style(s) and talker metadata have different OverlayModes. Maybe not all need to be a user available settings (but they could).

I've put comments of Should always be "overlay" mode? where I thought the option should be static.

Another (bad) idea would be to mark the origin of the data (maybe re-purpose/use TagOrigin) in the GenericMetadata.

lordwelch · 2024-03-02T23:21:21Z

You use the quirk on the commandline that an empty string value clears a list as a reason for why these options need to exist..... And then you explicitly say that none of the options you created should be used......

        md.overlay(self.config.Runtime_Options__metadata)  # Should always be "overlay" mode?

mizaki · 2024-03-03T00:29:50Z

You use the quirk on the commandline that an empty string value clears a list as a reason for why these options need to exist..... And then you explicitly say that none of the options you created should be used......
        md.overlay(self.config.Runtime_Options__metadata)  # Should always be "overlay" mode?

It depends on the answer to the question of keep using the quirk or not? If it is used then OverlayMode.overwrite for example is required for that reason (i.e. having to check if a list is a list). If, say, TagOrigin is used to demarcate if the data is from commandline, file name, etc. then OverlayMode.overwrite could be the same (and thus removed and do a copy). It could also be that

# finally, use explicit stuff
md.overlay(self.config.Runtime_Options__metadata)

Does not use the user available setting and is always OverlayMode.overlay with the list/set == str in whereas the other modes do not check.

Having the user choose for all four of the origins (file name, read style, commandline and talker) might lead to confusion?

lordwelch · 2024-03-03T01:08:33Z

It depends on the answer to the question of keep using the quirk or not

No. You have several different modes that you have created. When I said to remove them or asked why they exist you explicitly said that they existed because of the quirk and then you explicitly don't use them the only place the quirk happens. Remove the useless modes. It's a stupid quirk that probably shouldn't ever have been added in the first place.

If something needs to change about how metadata is merged from different sources that needs to change where the decision is made, in this case cli.py create_local_metadata. Creating an entire complicated system to facilitate a quirk is stupid. It was implemented as a tiny 7 line change in 539aac1 to allow unsetting the credits #587. If you can turn this into a 50 line change (minus tests) I'll merge it as is but this is over 700 lines changed, most of which is just to satisfy a stupid quirk that doesn't even get executed.

mizaki · 2024-03-05T01:07:26Z

I've squished and targeted what is currently already there. The overlay options will only apply to source (talker) information currently. So this option will become more relevant when the multi-read of metadata styles is done.

I have also renamed Comic Book Lover to Metadata Options and moved those that are to do with metadata into it.

comicapi/genericmetadata.py

lordwelch · 2024-03-10T19:49:21Z

comicapi/genericmetadata.py

            # otherwise, add it!
            else:
-                self.add_credit(c["person"], c["role"], primary)
+                cur_credits.append(Credit(person=c["person"], role=c["role"], primary=primary))


All of the credit appends need to do the same checks as self.add_credit

It might be better to merge them and then clear the current credits and just add them at the end

credits = assign_credit_func(cur_credits, new_credits) self.credits.clear() for credit in credits: self.add_credit(**credit)

I have kept it a return value so it's not different to the others. I've also removed the empty person to remove role.

As the add_credit is done to each md I did a dupe check so using add_credit isn't used again. I think it makes sense but if you want to still use it, I'll change it.

lordwelch · 2024-03-10T20:32:11Z

comictaggerlib/ui/settingswindow.ui

+                <string>Overlay - When the new source metadata has non-empty values, overwrite the current metadata
+
+Add Missing - Any values that are empty in the current metadata that are in the new source metadata, add those values
+
+Combine - Combine lists and use the new (non-empty) source metadata for all other values</string>


I don't really like the wording here. What do you think of

Overlay - Overlays new metadata on top of the original.
(Series=batman, Issue=1, Tags=[batman,joker,robin])
+ (Series=Batman, Publisher=DC Comics, Tags=[mystery,action])
= (Series=Batman, Issue=1, Publisher=DC Comics, Tags=[mystery,action])

Add Missing - Metadata missing from the original is added from the the new metadata.
(Series=batman, Issue=1, Tags=[batman,joker,robin])
+ (Series=Superman, Issue=10, Publisher=DC Comics, Tags=[superman,lois lane,lex luthor])
= (Series=batman, Issue=1, Publisher=DC Comics, Tags=[batman,joker,robin])

Combine - Same as Overlay except lists (tags, characters, genres, credits, etc...) are combined.
(Series=batman, Issue=1, Tags=[batman,joker,robin])
+ (Series=Batman, Publisher=DC Comics, Tags=[mystery,action])
= (Series=Batman, Issue=1, Publisher=DC Comics, Tags=[batman,joker,robin,mystery,action])

But now that's a lot more text....

How about making this a radio box so each item can have it's own description on the screen and we can make the example the tool tip?

Examples should make things easier to understand.

I'm thinking of a text box (on the right as there is space) that changes to reflect the current chosen or, a tabbed text box for the options.

This is kind of related but I think it makes sense to have a choice for the read styles and the source data (with commandline always in overlay with clearing of lists etc. and having primacy over read styles). So having two combo boxes makes the most sense then rather than radio.

This is what I went with. Could possible have it switch tab when a combo box is changed.

Read style and source info need to have similar text as they are the same operation but the metadata comes from a different place

What about covering both? Something like:

Source Info: Overlays the new 'source info' metadata on top of the current metadata as taken from the file name parser or read style (unless 'Overwrite metadata was selected) Read Style: Overlays subsequent metadata after the first read style from the next read style as set by the user

The read style text above probably needs some simplification but that general idea?

So we have the origin of the data "read", "source" and then we have "style" indicating there is some sort of style to the reading, then we have "info" which doesn't contribute any real meaning and doesn't follow the other control. They need to have consistent names. Imagine if on the front page we had this

That just adds confusion, because instead of keeping consistent names for the controls it uses completely different terms. I really care what terminology you use but it needs to be consistent. And if the name can indicate that "source" is from comicvine (or one of the others) then it needs a description as well (either as a tooltip or a label)

IIRC you said to use "style" for the read and modify when it's user facing so I did that. "Data Source" IIRC I changed because of the space it takes up in the tagger window. And then in the settings it's still "Metadata Source", so I think that's the root of the problem I made. In the auto-tag window the option is labelled Overwrite metadata so maybe removing the word "Source" from the tagger window and simply put "Metadata" is the answer?

The tool tip for both the label and combox for source info is:

The operation to perform when overlaying source data (Comic Vine, Metron, GCD, etc.)

and for read style(s):

The operation to perform when overlaying read styles (ComicRack, ComicBookInfo, etc.)

Are they okay as is?

And the general format of using the one tab to explain both "Metadata" and "Read Style(s)" is okay?

You're missing the point. That was just an example. I don't really care what terminology you use, it just needs to be consistent with the control it is next to.

I've changed it to "Data Source" to match the tagger window.

I've changed the description to this:

Read Style: Overlays all the none empty values of the read style metadata on top of the current metadata (i.e. file name parsing, other read style(s)). Data Source: Overlays all the none empty values from the data source (e.g. Comic Vine) on top of the current metadata (i.e. file name parsing, read style(s)).

I think now that with the wording there is no need for separate descriptions for "Read Style" and "Data Source"?

lordwelch · 2024-03-16T18:28:04Z

comictaggerlib/ui/settingswindow.ui

+                <item>
+                 <widget class="QPlainTextEdit" name="combineTextEdit">
+                  <property name="plainText">
+                   <string>Same as Overlay except lists (tags, characters, genres, credits, etc.) are combined.


This is not accurate with the current code anymore

How's this:

Combines lists (tags, characters, genres, credits, etc.) replacing any duplicates with the new record. All other non-empty values replace any current.

Is there a reason for the code to not match the description?

What am I missing?

Combines lists (tags, characters, genres, credits, etc.) replacing any duplicates with the new record. All other non-empty values replace any current.

I think that is correct? "replacing" maybe isn't the perfect word, "using the new value with any duplicate items"?

The current description says "Same as Overlay except for lists...." but, it isn't, can we just make the code match the description?

I'm with you now. It was a 50/50; change the description or change the code and I chose poorly.

If the command line empty string is taken out (presuming that command line metadata will always be used in overlay mode) then they look to do the same outside of lists/sets?

def assign_overlay(self, cur: Any, new: Any) -> Any: """Overlay - When the 'new' object has non-None values, overwrite 'cur'(rent) with 'new'.""" if new is None: return cur # An empty string can be used to clear a list or set via the commandline if isinstance(new, str) and len(new) == 0: if isinstance(cur, (list, set)): cur.clear() return cur else: return None elif isinstance(new, (list, set)) and len(new) == 0: return cur else: return new

def assign_combine(self, cur: Any, new: Any) -> Any: """Combine - Combine lists and sets. All non-None values from new replace cur(rent)""" if new is None: return cur if isinstance(new, (list, set)) and isinstance(cur, (list, set)): if len(new) > 0 and isinstance(new, list) and isinstance(new[0], Url): return list(set(new).union(cur)) return self.assign_dedupe(new, cur) else: return new

Apologies if I'm missing the obvious.

comicapi/genericmetadata.py

comictaggerlib/settingswindow.py

lordwelch · 2024-03-16T18:32:24Z

comictaggerlib/taggerwindow.py

@@ -1166,7 +1166,8 @@ def query_online(self, autoselect: bool = False, literal: bool = False) -> None:
                            description=cleanup_html(
                                new_metadata.description, self.config[0].Sources__remove_html_tables
                            ),
-                        )
+                        ),
+                        self.config[0].Metadata_Options__source_overlay,  # command line?


What's the question here?

That was a question I meant to ask: On the command line the styles from the file setting can be overridden but not saved. Should overlay do the same?

lordwelch · 2024-03-16T18:37:21Z

comicapi/genericmetadata.py

+            if len(new) > 0 and isinstance(new, list) and isinstance(new[0], Url):
+                return self.weblinks_dedupe(new, cur)  # type: ignore[arg-type]
+            return self.assign_dedupe(new, cur)


This fails when there are no new links and there is a current link. Probably a good test case.

comicapi/genericmetadata.py

mizaki · 2024-05-06T22:35:35Z

Only other possible change is tag_origin to a list?

…ty string will never make it to genericmetadata now

…etadata.

… window

…d lists message

mizaki · 2024-05-11T21:29:05Z

I've added the "lists" option. I feel like something is missing now but I have no idea what.

Two dropdowns with only two options seems too few!

mizaki · 2024-05-14T16:54:58Z

Thinking about "lists", perhaps it should be three options: merge, replace or ignore (leave original data)?

lordwelch · 2024-05-14T17:58:51Z

replace is the wrong word. It falls back to whatever the merge mode is

mizaki · 2024-05-15T17:59:53Z

"ignore" might be the final option needed for attributes and lists. That way a user can update only attributes or lists. That would bring lists to a select the same as attributes and bring attributes to three options as well.

Shall I add an ignore and convert "lists" options from a bool?

lordwelch · 2024-05-15T19:32:52Z

I don't see why someone would treat lists like that. Let's wait until someone has a usecase for it

lordwelch reviewed Feb 7, 2024

View reviewed changes

mizaki force-pushed the metadata_overlay branch from b788252 to 1c01bb4 Compare February 13, 2024 01:47

lordwelch reviewed Feb 14, 2024

View reviewed changes

mizaki force-pushed the metadata_overlay branch from 75c825b to ddbc5d8 Compare February 14, 2024 22:31

mizaki force-pushed the metadata_overlay branch from ddbc5d8 to 4f97a70 Compare February 16, 2024 01:01

lordwelch requested changes Feb 16, 2024

View reviewed changes

mizaki force-pushed the metadata_overlay branch from 60dccaf to 4de43d0 Compare February 26, 2024 01:09

mizaki force-pushed the metadata_overlay branch from 3203103 to 6063553 Compare March 5, 2024 01:00

lordwelch reviewed Mar 10, 2024

View reviewed changes

mizaki force-pushed the metadata_overlay branch from 6fb2d58 to 1eee20d Compare March 16, 2024 01:54

lordwelch reviewed Mar 16, 2024

View reviewed changes

mizaki force-pushed the metadata_overlay branch from 5f1b4f7 to b0cc000 Compare April 21, 2024 00:08

mizaki force-pushed the metadata_overlay branch 2 times, most recently from 67bb38a to 2072150 Compare May 6, 2024 21:58

mizaki added 15 commits May 11, 2024 01:56

Add OverlayMode options for read style and data source

8b0683f

Remove data to test empty string->None for series and issue as an emp…

93be16f

…ty string will never make it to genericmetadata now

Change settings menu overlay descriptions

b2f95fa

GUI overlay settings moved to internal namespace and CLI args added

ffb4efb

Add OverlayMode options for read style and data source

62240bf

Change settings menu overlay descriptions

94b94b7

GUI overlay settings moved to internal namespace and CLI args added

e9a592d

norm_fold out of loop for add_credit. Explicit overlay mode for CLI m…

2faac18

…etadata.

Move some overlay test data to testing/comicdata.py

71b79bd

Rename CBL option to Metadata

b761763

lordwelch rewrite

3d443e0

Separate list merge into a separate option (lordwelch)

a40438d

Rebase corrections

bada694

Add option for merge lists and fix saving overlay options in settings…

6c3b63a

… window

Remove combine overlay. Alter help messages in settings window and ad…

250d777

…d lists message

mizaki force-pushed the metadata_overlay branch from 7298a67 to 250d777 Compare May 11, 2024 21:26

lordwelch closed this May 11, 2024

lordwelch reopened this May 11, 2024

lordwelch closed this May 12, 2024

lordwelch reopened this May 12, 2024

lordwelch approved these changes May 14, 2024

View reviewed changes

lordwelch merged commit 250d777 into comictagger:develop May 18, 2024

Add overlay modes for metadata; overwrite, add missing etc. #598

Add overlay modes for metadata; overwrite, add missing etc. #598

Uh oh!

Conversation

mizaki commented Feb 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mizaki commented Feb 14, 2024

Uh oh!

mizaki commented Feb 14, 2024

Uh oh!

lordwelch commented Feb 14, 2024

Uh oh!

mizaki commented Feb 14, 2024

Uh oh!

lordwelch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mizaki commented Feb 15, 2024

Uh oh!

lordwelch commented Feb 15, 2024

Uh oh!

mizaki commented Feb 16, 2024

Uh oh!

mizaki commented Feb 16, 2024

Uh oh!

lordwelch commented Feb 16, 2024

Uh oh!

mizaki commented Feb 16, 2024

Uh oh!

lordwelch commented Feb 16, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lordwelch commented Feb 17, 2024

Uh oh!

mizaki commented Feb 17, 2024

Uh oh!

mizaki commented Mar 2, 2024

Uh oh!

lordwelch commented Mar 2, 2024

Uh oh!

mizaki commented Mar 3, 2024

Uh oh!

lordwelch commented Mar 3, 2024

Uh oh!

mizaki commented Mar 5, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment