Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add alternative author roles properties #434

Merged
merged 20 commits into from
Dec 7, 2023
Merged

Conversation

maxlath
Copy link
Member

@maxlath maxlath commented Sep 4, 2023

addressing #202
depends on inventaire/inventaire#696

This PR does a few things:

  • add the possibility to edit alternative author role properties for works and series, depending on their P31 value (ex: add an illustrator (P110) to a comic book (P31:Q1004) or manga (P31:Q8274))
  • add the possibility to move a claim between this different author role properties. In particular, the possibility to move an existing P50 to a more precise role will come handy, as all our local entities have been using P50 only

Notable implementation details:

  • the use of the recently enabled (17f9fd0) top-level await to import all properties values at once (see server-side PR for endpoint changes) and then import those values sync in consumer modules: this pattern could become handy to share this kind of configuration between the server and the client.

@maxlath maxlath force-pushed the 202-add-author-properties branch 2 times, most recently from f75199e to 47a60f8 Compare September 6, 2023 12:53
@maxlath maxlath changed the title [WIP] add alternative author roles properties add alternative author roles properties Sep 7, 2023
@maxlath maxlath requested a review from jum-s September 7, 2023 11:37
@nclm
Copy link

nclm commented Sep 7, 2023

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
Copy link
Member Author

maxlath commented Sep 7, 2023

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
Copy link

nclm commented Sep 7, 2023

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
Copy link
Contributor

jum-s commented Sep 20, 2023

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
Copy link
Contributor

jum-s commented Sep 20, 2023

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
Copy link
Contributor

jum-s commented Sep 30, 2023

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
Copy link
Contributor

jum-s commented Oct 4, 2023

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
Copy link
Member Author

maxlath commented Nov 2, 2023

@jum-s

(Answering in review threads would be more convenient)

Cannot have duplicate keys in a keyed each

should have been fixed by 9fb244a

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:

  • keeping the P50 slot but disabled, with a message encouraging to use more specific roles
  • replacing the generic property editors by an even more ad-hoc "authors" editor, with a list of all contributors and the possibility to add their roles in place, that could look something like this :

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
Copy link
Member Author

maxlath commented Nov 3, 2023

@nclm after 2db5845 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
Copy link
Contributor

jum-s commented Nov 6, 2023

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.

by introducing a dedicated symbol, which is then used to hide the moved claim ClaimEditor
without affecting other instances of the ClaimEditor in the PropertyClaimsEditor {#each} block
(as it is keyed by index and not value)
by hiding the selector behind a button. This as the advantage to
hide the repetition between the property label (ex: "author") and the selector default option ("author" again)
@maxlath maxlath requested a review from jum-s November 26, 2023 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants