Third step secrets management - [merged] #48

Closed cswendrowski closed 2 years ago

In GitLab by @adrien.schiehle on Nov 17, 2021, 10:29

_Merges third_step_secretsmanagement -> secrets

Next step : Secret management.

Only Storyteller seeds are managed for now. I've chosen to split those seeds by checking if there is some <h3 ...> inside of it. Each h3 indicating the start of a new secret. It will allow GMs to reveal secrets step by step for a given article.

<h3> inside what we retrieve are what we write as [h2] inside BBCode.

It works like that :

When importing article, we store one property inside world-avil.secrets flag each secret.
GMs can toggle each secret insde the Journal Entry. Or toggle all in one go inside the WorldAnvilBrowser.
The WorldAnvilBrowser indicates which article contains secrets.
Players can't see secrets which has not been revealed.
Players see revealed secret inside Journal Entries the same way they see other sections.

In GitLab by @adrien.schiehle on Nov 19, 2021, 09:56

added 1 commit

c1afa047 - Refreshing WorldAnvilBrowser after toggling secret

Compare with previous version

changed target branch from categories to secrets

It would be nice to use .svg icons instead of .png for smaller filesize. We can also consider using font awesome icons very easily which are already included in Foundry VTT.

In GitLab by @adrien.schiehle on Nov 19, 2021, 13:56

added 4 commits

c1afa047...ba8c1e28 - 3 commits from branch foundrynet:secrets
3ccd6980 - Merge branch 'master' into third_step_secrets_management

Compare with previous version

Document as @enum {string}

It might be more direct to store a module-level cached reference to the WorldAnvilBrowser instance instead of searching through ui.windows to find it.

I think I probably need to chat with you some to understand the high-level context of the change. I'm not very familiar with secrets on World Anvil so I want to understand them a little better before reviewing the code to implement them on the Foundry side.

In GitLab by @adrien.schiehle on Nov 19, 2021, 14:09

I'm not using the full extend of what can be used inside World-Anvil.

It has a secret section which visibility for only some subscriber groups. Since (for now), I don't intend to correlate foundry players and logins inside world-anvil, I've chosen to not use them inside the import. (And in fact, they are not available is Dimitri's current api)

I'm only using this (available for each article):

I chose to split its content in multiple secrets, by checking the use of [h2] in it. (Two secrets on previous screenshot)

In GitLab by @adrien.schiehle on Nov 19, 2021, 14:14

Commented on module/journal.js line 51

Something like that ?

Hooks.once('ready', () => {

    game.worldAnvilBrowser = new WorldAnvilBrowser();
});

In GitLab by @adrien.schiehle on Nov 19, 2021, 15:32

Commented on module/journal.js line 51

changed this line in version 5 of the diff

In GitLab by @adrien.schiehle on Nov 19, 2021, 15:32

added 2 commits

6694d934 - Similar display as <section class="secret"
f6ddba87 - module-level cached reference to the WABrowser

Compare with previous version

In GitLab by @adrien.schiehle on Nov 22, 2021, 11:31

I'm using a background image with a left padding. And I use different icons (and colors) depending on the secret state :

.wa-section.wa-secret {
    //...
    padding: 0 5px 0 30px;
    background-image: url('icons/hidden_secret.png');
    background-repeat: no-repeat;
    background-position-x: left;
    background-position-y: center;
    background-size: 28px;
}

.wa-section.wa-secret.hidden {
    background-image: url('icons/hidden_secret.png');
}

.wa-section.wa-secret.hidden:hover {
    background-image: url('icons/hidden_secret_hover.png');
}

Can svg icons have colors ?

I can also try to switch to fontawesome icons, but it would means adding text instead of just using a background image. It could work, but the code will become for more complex. (adding a left colum on the fly for each secret section)

I can try if you want. But I'm not convinced.

In GitLab by @adrien.schiehle on Nov 23, 2021, 20:06

added 1 commit

ea14829a - Custom secrets transfered to a distinct module

Compare with previous version

In GitLab by @adrien.schiehle on Nov 23, 2021, 20:07

The custom secret management has been transfered here : https://gitlab.com/adrien.schiehle/world-anvil-secrets

In GitLab by @adrien.schiehle on Nov 23, 2021, 20:09

added 1 commit

1a0888c3 - unused icons

Compare with previous version

In GitLab by @adrien.schiehle on Nov 23, 2021, 20:13

added 1 commit

1a71291d - inexact comment and unused code

Compare with previous version

In GitLab by @adrien.schiehle on Nov 25, 2021, 21:06

added 1 commit

fa258d8b - Compatibility with world-anvil-actors

Compare with previous version

I don't think we should specifically pre-suppose that the user of this hook will only care about flags. They may want to do other things as well. This hook should more generally handle pre-processing of content before the entry is updated.

I would prefer something like:

Hooks.callAll(`WAUpdateJournalEntry`, entry, content);

We should have a corresponding hook for cases where we are first creating a journal entry:

Hooks.callAll(`WACreateJournalEntry`, createData, content);

I don't think this is the right hook design. I feel that we should do our default parsing first and then pass the parsed result to a hook to modify/change if they want. Doing hooks first and then merging it is not the right design pattern.

Dealing with fromHooks makes the rest of our parsing function more complicated which is not desirable.

This is where we should be calling the hook

const parsedData = {
  html.
  img: image,
  waFlags: waFlags
}
Hooks.callAll("WAParseArticle", article, parsedData);

This section needs a line of code commentary to make it clear what/why we are doing this.

Code style, please use parentheses to enclose composite logical conditions:

game.folders.find(f => (f.data.type === "JournalEntry") && (f.getFlag("world-anvil", "categoryId") === category.id));

I'm not sold on displaying in the UI whether an article has secrets or not. We don't do that in the main Journal Entry UI so I don't know why it's needed here either.

I don't really like using css id here since we maybe cannot guarantee that it is unique across all UI windows? This feels like it would be better handled as a data element like data-section-id="${id}"

Overall this looks better, but I think there is still a little bit too much complexity due to your handling of hooks. Please see my comments and let's discuss in Discord if you have any questions/feedback. Thanks!

requested review from @aaclayton

In GitLab by @adrien.schiehle on Nov 26, 2021, 11:13

Commented on templates/journal.hbs line 62

Well, I'm in fact using it as a teaser for the world-anvil-secrets module :angel:

You're right saying it has no meaning in the core module. When hovering it, it says :

This article has some secrets. You will need a complementary module if you want to toggle secrets

If you think it's uncalled for, I will remove it.

In GitLab by @adrien.schiehle on Nov 26, 2021, 11:17

Commented on module/framework.js line 116

I agree on this. I was too much focused on my current needs and didn't do something which could be used in other circumstances. I will work on that

In GitLab by @adrien.schiehle on Nov 26, 2021, 11:27

Commented on module/framework.js line 161

I didn't want to have to parse the html content to remove already added data. It seems far less complicated at first but it may fall in the same problem as your previous comments.

If I want to keep to totally generic, I will need to do as you say.