-
Notifications
You must be signed in to change notification settings - Fork 11
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
fix: protocollib as soft dependency #9 #10
Conversation
Ah oui et j'ai changé la tâche |
Pourquoi toi tu ne test pas ?
Je ne comprend pas ? ProtocolLib n'est pas un plugin que MineWebBridge dépend. |
src/main/java/fr/vmarchaud/mineweb/bukkit/MinewebCommandSender.java
Outdated
Show resolved
Hide resolved
C'est juste pour éviter d'avoir une énorme erreur dans la console et afficher une erreur compréhensible par l'user et en sois c'est pas con. |
ProtocolLib est indirectement nécessaire car MineWeb utilise leurs utilitaires (voir fr/vmarchaud/mineweb/bukkit/BukkitCore et fr/vmarchaud/mineweb/bukkit/BukkitNettyInjector) Pour les tests, ce n'est pas que je ne veux pas mais que je n'ai pas de site avec lequel le tester et n'ait aucunement envie d'installer tout le nécessaire pour le faire actuellement. Théoriquement ça devrait fonctionner pour BungeeCord je n'ai pas d'erreur en lançant un serveur avec le dernier build. |
D'accord, ouais, dans ce cas, je vois & je suis d'accord. Je vais voir si je peux le testée avant Janvier (mais je ne promet rien) |
C'est pour cela qu'il y a 2 plugins, lorsque le plugins protocolibs est installé en même temps que mineweb cela crée une erreur il faut donc prendre le second plugin même si pour moi le mieux serait de rend dependant le plugin mineweb avec protocol libs pour limiter les demandes de support lorsque les personnes ne lisent pas la doc et ne savent pas que lorsque protocolibs est installé il faut une autre version du pl mw |
Selon moi, cela dépendrait toujours si cela en vaut vraiment la peine. |
J'ai fix les conflicts (via un ptit rebase) et comme ça la diff est lisible mtn.
Tu parlais des Github actions? Si oui je sais, je vais regarder Et c'est une bonne idée de mettre ProtocolLib en soft dependency et d'émettre une erreur quand il est nécessaire. Les gens seront pas forcés de l'installer si ils utilisent la commande |
yes boss |
J'ai utilisé Java 8 |
Bonjour, |
# Conflicts: # .github/workflows/gradle.yml # src/main/java/fr/vmarchaud/mineweb/bukkit/BukkitCore.java
J'ai commit un fix pour l'erreur de @jimmyr57000 et j'ai réglé un potentiel problème avec Java 11 (afd56b7). |
Hey, hey ! :) |
Pour moi mon review est pas résolue, donc non il ne faut pas merge encore, tu es sur la version sans protocollibs si protocolibs est installé il crash, donc il ne faut pas le mettre en softdepends, il faudrai le mettre sur la version no-injector, ensuite sur cette version on peut mettre le port du serveur contrairement a l'autre version no injector :p |
@nivcoo Maintenant, les deux versions peuvent être merge dans une seule qui est celle-là. En effet si tu veux pouvoir bind le port de ton serveur il te faudra installer protocollib mais si tu ne veux pas l’installer tu pourras utiliser la commande pour bind un port custom |
Oui j'ai eu ma réponse sur discord j'avais pas vu les modif nécessaire, et de base c'est pas l'inverse ? il ne te faut pas protocolibs si tu veux bind le port du serveur ? Il me semble que c'est ca, dans l'ancienne version si tu avais la no-injector donc avec protocolibs tu pouvais plus bind le port du serveur |
if (config.getPort() == null) | ||
|
||
if (config.getPort() == null) { | ||
if (Bukkit.getPluginManager().isPluginEnabled("ProtocolLib")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Si l'erreur est
The bridge requires ProtocolLib to run on server's port
pourquoi mettre
if (Bukkit.getPluginManager().isPluginEnabled("ProtocolLib")) {
Ensuite justement si le port vaut null alors il ne faut pas protocolibs libs, Enfin maintenant que les 2 sont splits ca ne change pas le systeme d'avant : Sans protocolibs nous pouvons mettre un port custom + port du serveur mais avec(donc la version no-injector) il etait impossible de mettre le port du serveur : https://pics.edensky.fr/2019-12/23/18-36_iUxm.png
C'est bon je viens de tester sur le bungee cela fonctionne bien , jimmy avait testé sur le bukkit donc c'est ok :p |
Bukkit:
MAJ 24/12/2019 BungeeCord:
Divers:
Voilà, j'ai terminé les essais de mon côté je valide 💯 |
Parfait ! 💯 |
Je pense pas qu'il puisse y avoir un probleme avec ca, mais si tu veux je peux tester d'avoir port du serveur pour le bungee et serveur |
Fait le test perso flemme ! |
Je viens de tester il n'y a pas de soucis |
Tout le monde est d'accord pour merge alors? |
Go pas de soucis |
Oui et comme ça je vais pouvoir faire mon ajout |
Tout est parfait sauf : Je n'ai plus utilisé le port par défaut du serveur (25565 Bukkit, 25577 pour Bungee) (aucune idée, si cela avait été implémenté) Sinon, tout le reste fonctionne correctement, si on précise un port autre que celui par défaut. Et check @Shyrogan : |
@MaximeMichaud C'est pas dramatique pour le moment, c'est mener à disparaître dans les futurs versions de Java mais c'est pas encore le cas donc profitons en. |
Fixes: