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

feat: allow addons to add wiki pages to items #3937

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

ybw0014
Copy link
Contributor

@ybw0014 ybw0014 commented Aug 6, 2023

Description

Nowadays there are more and more Slimefun addons, but only Slimefun items can have the wiki item in the recipe display. Some addons already have wiki, but players may not be able to know the wiki's existence, unless the developer add the wiki link via FlexItemGroup or using other ways. I think the wiki page item should be available to all the items that have wiki for them.

Tested with Sefiraat/Networks#152.

Proposed changes

  • Add getWikiURL() in SlimefunAddon to provide a base URL for addon wiki.
  • Add getWikiPage(String) in SlimefunItem to allow items to have wiki pages.
  • Deprecate addOfficialWikipage in favor of getWikiPage.
  • Display a different wiki icon for all addons.
  • Move the code that setups wiki from wiki.json to WikiUtils.

Related Issues (if applicable)

N/A

Checklist

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
  • I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.20.*).
  • I followed the existing code standards and didn't mess up the formatting.
  • I did my best to add documentation to any public classes or methods I added.
  • I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
  • I added sufficient Unit Tests to cover my code.

@ybw0014 ybw0014 requested a review from a team as a code owner August 6, 2023 01:11
@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2023

Pro Tip!
You can help us label your Pull Requests by using the following branch naming convention next time you create a pull request. ❤️

Branch naming convention Label
feature/** 🎈 Feature
fix/** ✨ Fix
chore/** 🧹 Chores
api/** 🔧 API
performance/** 💡 Performance Optimization
compatibility/** 🤝 Compatibility

If your changes do not fall into any of these categories, don't worry. You can just ignore this message in that case! 👀

@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2023

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: 69df2b5

https://preview-builds.walshy.dev/download/Slimefun/3937/69df2b59

Note: This is not a supported build and is only here for the purposes of testing.
Do not run this on a live server and do not report bugs anywhere but this PR!

@JustAHuman-xD JustAHuman-xD added 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. 🔧 API This Pull Request introduces API changes. labels Aug 6, 2023
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea but limiting the addon to be under one url + a placeholder imo isn't ideal. If there was a way to add a full link to an item on top of this I think it would be a good addition

@J3fftw1
Copy link
Contributor

J3fftw1 commented Aug 6, 2023

I like the idea but limiting the addon to be under one url + a placeholder imo isn't ideal. If there was a way to add a full link to an item on top of this I think it would be a good addition

Yea i agree with this, maybe we can have the wiki link be defaulted to the repo as how you have it now but allow it to be overridable with custom links.
this will allow sites like gitbook to be used

@ybw0014
Copy link
Contributor Author

ybw0014 commented Aug 6, 2023

For anyone would like to test this build using the preview jar, here is the modified Networks that utilized the methods added in this PR.
Download link: https://cloud.ybw0014.net/s/PwTr (my own cloudreve instance)
If you want to compile it by yourself, the PR link is here: Sefiraat/Networks#152

Copy link
Member

@WalshyDev WalshyDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fully opposed to giving a way to provide wiki links for addons.

However...

Arbitrary links doesn't feel good. Domains will be bought, not renewed and snatched by others. I think we need to figure out the right approach to this rather than just throw this out there.
We do not want to be the ones responsible for someone getting malware on their system or seeing adult material due to a snatched domain.

@ybw0014
Copy link
Contributor Author

ybw0014 commented Aug 7, 2023

I'm not fully opposed to giving a way to provide wiki links for addons.

However...

Arbitrary links doesn't feel good. Domains will be bought, not renewed and snatched by others. I think we need to figure out the right approach to this rather than just throw this out there. We do not want to be the ones responsible for someone getting malware on their system or seeing adult material due to a snatched domain.

Maybe add a warning message before sending the link in the chat?

@J3fftw1
Copy link
Contributor

J3fftw1 commented Aug 7, 2023

Warnings don’t mean much and are almost always ignored

@ybw0014
Copy link
Contributor Author

ybw0014 commented Aug 7, 2023

Warnings don’t mean much and are almost always ignored

It is the player's choice to ignore the warning.

@J3fftw1
Copy link
Contributor

J3fftw1 commented Aug 7, 2023

i dont think this is the right approach we will still be kept accountable if someone gets a virus or something else.
I do really like this feature, but i do agree with Walshy completely

@StarWishsama
Copy link
Contributor

Maintaining a list of trusted wiki domain names could be a compromise, since no official addon wiki provided.

@Phoenix-Starlight
Copy link
Member

Phoenix-Starlight commented Aug 7, 2023 via email

@J3fftw1 J3fftw1 closed this Aug 7, 2023
@J3fftw1 J3fftw1 reopened this Aug 7, 2023
@J3fftw1

This comment was marked as off-topic.

@PhoenixCodingStuff
Copy link

I'm not fully opposed to giving a way to provide wiki links for addons.

However...

Arbitrary links doesn't feel good. Domains will be bought, not renewed and snatched by others. I think we need to figure out the right approach to this rather than just throw this out there. We do not want to be the ones responsible for someone getting malware on their system or seeing adult material due to a snatched domain.

Sf could have a list of whitelisted links, GitHub, gitbook and other docs hosting platforms. In addition to a warn that the files hosted are not officially by slimefun and made by the addon developer if that makes sense.

1 similar comment
@PhoenixCodingStuff

This comment was marked as duplicate.

@ybw0014
Copy link
Contributor Author

ybw0014 commented Dec 31, 2023

Minecraft has its own warning system already inplace for external links, If that fires im fine with this being in place.

Slimefun displays a message that contains the link when you click on the wiki button, so that will be fired.

# Conflicts:
#	src/main/resources/languages/en/messages.yml
@ybw0014
Copy link
Contributor Author

ybw0014 commented Jan 6, 2024

As I didnt get any response for panel thing, I just merged commits and resolved merge conflicts.
As Jeff mentioned, Minecraft has its warning system, as we just send the link to users and users will be warned when they click on a link in the chat.

@TheBusyBot TheBusyBot removed the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Jan 7, 2024
@J3fftw1
Copy link
Contributor

J3fftw1 commented Jan 7, 2024

Have you tested this with an addon that uses this?
Would love to see the button working.

@ybw0014
Copy link
Contributor Author

ybw0014 commented Jan 7, 2024

Have you tested this with an addon that uses this? Would love to see the button working.

I tested this earlier with Networks (Sefiraat/Networks#152), now I'm gonna test this again.

@ybw0014
Copy link
Contributor Author

ybw0014 commented Jan 7, 2024

Just tested and it still works. However, the item name seems to be a bit long, should we move some parts into item lore?

image
image

@J3fftw1
Copy link
Contributor

J3fftw1 commented Jan 7, 2024

Yea let’s just call it wiki

@PhoenixCodingStuff
Copy link

Agreed with Jeff, the item name can be Wiki to make it smaller while the error message is shown in chat.

@ybw0014
Copy link
Contributor Author

ybw0014 commented Jan 7, 2024

I changed the wiki button name to 'View this item on the %addon% wiki', where %addon% will be replaced by addon name.
And add a line to show if it is official Slimefun wiki, or third party wiki.

@ybw0014
Copy link
Contributor Author

ybw0014 commented Jan 7, 2024

image
image

@ybw0014 ybw0014 changed the title Allow addons to add wiki pages to items. feat: allow addons to add wiki pages to items Jan 8, 2024
@TheBusyBot TheBusyBot added the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Jan 18, 2024
# Conflicts:
#	src/main/java/io/github/thebusybiscuit/slimefun4/implementation/setup/PostSetup.java
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we stop another plugin changing the wiki pages of another?

If an addon or just random plugin wanted to they could loop through the items and change the wiki page right?

And if they did that to the core slimefun items it would still appear as "official" even if it isn't

Or you could make a slimefun item have an "official" wiki, if you register the item with the slimefun instance (which you shouldn't do but you could to trick this system)

@ybw0014
Copy link
Contributor Author

ybw0014 commented Mar 21, 2024

good point, maybe once the item has a wiki page, the wiki page link cant be changed further.

@TheBusyBot TheBusyBot removed the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Mar 22, 2024
@ybw0014 ybw0014 requested a review from JustAHuman-xD October 16, 2024 20:57
@ybw0014
Copy link
Contributor Author

ybw0014 commented Oct 16, 2024

Its been a while.
I added a state check on addCustomWiki so the wiki page cannot be added once it is already set.
Also, I use pattern match to check if the wiki URL is from official Slimefun wiki.

I wonder if I should remove addOfficialWiki since I don't think any addon would use this.

@ybw0014
Copy link
Contributor Author

ybw0014 commented Oct 16, 2024

Tested again with the modified Networks.

@ybw0014 ybw0014 requested a review from WalshyDev October 16, 2024 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔧 API This Pull Request introduces API changes. 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants