inventaire / inventaire-client

webapp coupled to the inventaire server :books:
https://inventaire.io
48 stars 16 forks source link

add alternative author roles properties #434

Closed maxlath closed 8 months ago

maxlath commented 11 months ago

addressing #202 depends on https://github.com/inventaire/inventaire/pull/696

This PR does a few things:

Notable implementation details:

nclm commented 11 months ago

Hello, just chipping in to make sure that the terminology is the right one :)

A comic book doesn’t usually have an “illustrator”, it has an “artist” (“dessinateur”⋅ice in French) and a “writer” (“scénariste” in French). Both of them are commonly called the “authors” of the book.

It would be incorrect to use P110 for the artist. Checking Wikidata for various comic books, it is indeed P50 that is used for both artists and writers. There are some comic books related properties such as letterer (P9191), inker (P10836), penciller (P10837), colorist (P6338), but I believe we are meant to use P50 for artists and writers. “Illustrator” just not the right word and not the same profession or role. We could argue that there should be “comic artist” and “comic writer” properties in Wikidata, but that’s another subject.

“Illustrators” are people making illustrations alongside a text, such as in some novels for instance, and other kind of works where the illustrations are not part of the main text. So the books that have “illustrators” are rarely or never comic books.

Moreover, sometimes there are illustrators that are just for an edition of a work: often, a classic text might be published many times with a different illustrator each time (and some editions without illustrations). Some other times, there might be a main illustrator that worked alongside the author and their illustrations are in all or most editions.

maxlath commented 11 months ago

P110 use to be the recommended property for "dessinateur⋅ice" (as shown by "dessinateur" being one of P110 French aliases), possibly with a qualifier, but there is now indeed P10837 that was created for that exact purpose, so that seems to be the way to go :)

nclm commented 11 months ago

Comics (often US) only mention a “penciller” if there is also a separate “inker”. I don’t think we should use “penciller” for most comics, which have the same artist doing both.

It’s also not completely correct to use “illustrateur⋅ice/dessinateur⋅ice” for the artist if there is no special “scénariste” role for the writer. Both are “authors”, so it isn’t valid to mark only the writer as so. Note that there is also an growing movement to consider the colorist as one of the authors.

Just want to make sure we use the right semantic properties :) I do think this PR is a good idea, great to have the choice of the author role! I have been missing “illustrator” for several books, but especially for specific editions of novels.

jum-s commented 11 months ago

When one create a new work, one choose Type: Comics, then P50 author slot disappear. But when one creates a new work, one fills the author P50 slot, one chooses Type: Comics P31:Q1004, then P50 author slot is still on.

It is understandable but slightly confusing to remove the author P50 slot. Maybe i dont have precise information about whose role the author exactly have, but i would like to have the possibility to still fill the author P50 slot (?)

[Edit] This P50 disappearance also happens after moving a human entity from author to (ie.) colorist, making it impossible to fill the author slot anymore.

[Edit2] Also related, when i edit an existing work (with no screenwriter/colorist/etc slots fields) with type "Comics" to type "Written work", The P50 author slot should be displayed. Same happens vice-versa, screenwriter/colorist/etc slots should be displayed

jum-s commented 11 months ago

When one create a new work, one fill the author P50 slot, one choose Type: Comics P31:Q1004, one changes from author to screenwriter with the little menu below the statement value Then an error is thrown:

[Cannot have duplicate keys in a keyed each: Keys at index 1 and 5 with value 'wdt:P58' are duplicates
validate_each_keys@webpack-internal:///./node_modules/svelte/src/runtime/internal/each.js:147:10
update@webpack-internal:///./app/modules/entities/components/editor/entity_create.svelte:502:72
update@webpack-internal:///./app/modules/entities/components/editor/entity_create.svelte:839:42
update@webpack-internal:///./app/modules/entities/components/editor/entity_create.svelte:432:15
update@webpack-internal:///./app/modules/entities/components/editor/entity_create.svelte:253:16
update@webpack-internal:///./app/modules/entities/components/editor/entity_create.svelte:944:16
update@webpack-internal:///./node_modules/svelte/src/runtime/internal/scheduler.js:133:30
flush@webpack-internal:///./node_modules/svelte/src/runtime/internal/scheduler.js:93:11
]
jum-s commented 10 months ago

Small feature suggestion: when an entity is "wrongly" associated with a work type (ie. a screenwriter for a written work), today if one moves it to author, then the screenwriter disappears. Which is fine if the user knows they are cleaning up the entity. But if they're not, it would be nice to suggest (ie. through a flash/infobox) that there is something smelly on this entity (à la Wikidata, with the tiny flag of the values constraints). It would be a nice little feature useful to this PR, that could then be generalized slowly on other cases.

[non-blocking, but to move in an issue]

jum-s commented 10 months ago

When one edits an existing "written work" with an author and an screenwriter, aka with claims as such:

  "claims": {
    "wdt:P31": [
      "wd:Q47461344"
    ],
    "wdt:P58": [
      "inv:0380fed910ff9a4bb8cc319031cd264a"
    ],
    "wdt:P50": [
      "inv:0835b4d395020e8753e296937cb0b4bc"
    ]
  },

Then one changes P31 to Comics (wd:Q1004), then one changes the author role to screenwriter. An error says

PossiblyUnhandledRejection: entity.claims[currentRoleProperty] is undefined

onRolePropertyChange@webpack-internal:///./app/modules/entities/components/editor/select_author_role.svelte:293:60
onChange@webpack-internal:///./app/lib/svelte/svelte.js:37:3
instance/$$self.$$.update@webpack-internal:///./app/modules/entities/components/editor/select_author_role.svelte:440:65
maxlath commented 9 months ago

@jum-s

(Answering in review threads would be more convenient)

Cannot have duplicate keys in a keyed each

should have been fixed by 9fb244a44

entity.claims[currentRoleProperty] is undefined

should have been fixed by ee605b9

It is understandable but slightly confusing to remove the author P50 slot

The idea is to encourage to directly enter the specific role. I can think of 2 alternatives, not sure which one is best:

B

It would be a nice little feature

I would rather keep that for a later improvement, possibly in another PR, after the main mechanism has been validated, to keep this PR small

maxlath commented 9 months ago

@nclm after 2db5845fe we now would have the following properties:

const mostlyTextWorksRoles = [
  'wdt:P50', // author
  'wdt:P110', // illustrator
]
const drawedWorksRoles = [
  'wdt:P58', // scenarist
  'wdt:P6338', // colorist
  'wdt:P9191', // letterer
  'wdt:P10836', // inker
  'wdt:P10837', // penciller
]

Any change suggestion?

jum-s commented 9 months ago

Answering in review threads would be more convenient

Yeah, this happens when i dont have located a specific line to address the comment. Duly noted though.