[Request] Remove FastOpenLinksAndFolders code
altrisi opened this issue ยท 8 comments
Hi! Would it be possible for you to remove the code from FastOpenLinksAndFolders from your mod?
I don't like it in another mod like this, so I'm asking if it would be fine for you to remove it.
TL;DR is the above, but if you want to read it here's more of my reasoning/motivations/concerns/ramblings and me writing too much (as I usually do):
One of the reasons is that the code on FastOpenLinksAndFolders doesn't fix any bug. As mentioned in the mod page, it offthreads the wait time to another thread, but that cannot fix any crash with how the code is handled in there, that bug will still happen, just in another thread. In some scenarios it may be converted into a less useful stacktrace, but it could also cause thread locking indefinitely if I read the bug report you linked correctly.
I also have an assumption about why it was added regardless of the above which I think it's awful, but it's nothing more than an assumption so it doesn't drive me that much.
Another reason is that the quality of the implementation has suffered a lot (IMO). Your implementation will cause another mod injecting nearby to silently fail in a completely untraceable way, given their method will be injected, just you will return before any custom logic applies, while in the original implementation Mixin will provide a full error that will tell the modder exactly what's going so they can at least debug the issue. I also don't feel comfortable with the testing that is done and how the way you pasted it made a release of yours crash. For me I think these changes also make it so when the very few people that will actually look at the source file see it as if my code causes issues, when it's actually the changes to the source or the build system differences.
More reasons are that I don't like it being in a big mod when such small feature could have any silent failure that would go unnoticed (given a mod/feature can not work without hard crashing, to give you an example the lazydfu release doesn't work on recent snapshots, but it doesn't crash so it's more difficult to notice, other than because of the slowdown of course), and I like maintaining my mod anyway and can ensure that it keeps working in newer versions I think better, especially since given I analysed the vanilla and other mods code and wrote the implementation I know exactly how it works.
I also believe in what Fabric has encouraged since the start: Modularity. Even FAPI is made out of lots of smaller modules instead of one big mod with everything, but it seems like debugify is trying to do exactly that, just with other mods instead of features.
Another thing that I'm not sure and don't think that much about (given I'm not a lawyer, this is not a legal threat or anything but mostly just curious if it's correct), but from the license I think you have to provide information about the original source (the mod) and the license in your mod, and you aren't anywhere in the binary (the binary has absolutely no reference about code being from the mod, and most users are never going to see anything but it), and the only license notice is hidden away in a comment in a single source file (with no link too). Also mentions:
- Conveying Modified Source Versions.
...
d) If the work has interactive user interfaces, each must display Appropriate Legal Notices; however, if the Program has interactive interfaces that do not display Appropriate Legal Notices, your work need not make them do so.
"The program" (FastOpenLinksAndFolders) implements no interfaces by itself for those, but it does display its name, license and author via interfacing with Fabric's metadata system and therefore ModMenu, but you aren't displaying it anywhere although you do have "interactive user interfaces".
After all of this, my issue isn't really a license thing. However, the reason I use open source licenses is to help others to learn from my code, provide trust so you know what you're running, and so in case I ever abandon the project it can be continued (and it's not abandoned, I still work on it when I'm bored, regardless if it's "small"). It's not my intention (even if it's technically allowed I guess) to allow for all my code to be pasted in another mod that makes it as if it had no relation to me anymore.
One last thing is that FastOpenLinksAndFolders is a mod that's a bit special for me. It was my first mod I ever published, even if it took literal months from coding the mod to making the CF page, and more months from the CF page to actually uploading the mod. I was just always thinking no one would care about it and it would be awful (I always underestimate myself/my work) so I wasn't uploading it, but the response has been quite different, which helped me a lot to believe more in what I do. I'd not like it to just be shadowed inside a mod where no one will know what it actually is.
Another TL;DR, this is just a petition where I'd thank you if you removed my code from your mod, nothing else.
Thanks!
Oh, and also I've been playing with making my mod a multiloader jar, so it would also work on Forge, mostly as a challenge to get it without using any Forge/Forge specific/Fabriforge tooling, and as far as I know (I haven't looked a lot at it though) you don't implement any compatibility thing on Forge, so it could cause you compatibility issues if someone tried to run both on Forge.
I don't work a lot on this, but I already made myself a POC with the mod running on Forge.
TL;DR: sure, I'll replace with actual bug fix (see last paragraph for explanation) but idk what to do because running on separate thread does help a lot
Hi. Thank you for informing me of your concern. I completely understand.
First of all, sorry about the licensing issue. I generally only read the github summary of licenses. I will correct this for all mods I may have used code from and add it to the in-game description. Though I don't understand what you mean about the lack of a link to FOLAF in the source, it is right there.
About the bad implementation, this implementation is a workaround not the desired method, Overwrite, Architectury Loom has trouble remapping mixin overwrites on forge (architectury/architectury-loom#77) so I was forced to use inject.
About the bad testing, I know I'm bad with testing and I've heard you extensively test even small changes. Usually, it's just Forge I neglect to test on as it only covers about 10% of Debugify's userbase. The crash actually led to me discovering previously mentioned bug in arch.
I'd be happy to remove your code and replace it with a mixin that actually fixes said bug. Kinda an oops on my part, didn't properly look into the bug report. Back in the early days of Debugify I was prioritising quantity over quality. Though I don't know how many choices I would have because running this task on another thread does free up the render thread to continue running the game.
Let me know what you think.
Oh no, the link to the mod is there in the source, yes, I meant it's not anywhere in the binary, or a link to/copy of the license (not mod), which (I think?) is something the license needs.
About the arch bug, Ik, I've mentioned it's because of the build system, I know it's not completely your fault, but that doesn't mean it cannot cause issues.
About testing I'm still not convinced, given according to the issue it was on fabric (if I read it and remember it right, was a long time ago).
Also small mention about the bug you reference with this (given I've looked into it to see if I had actually fixed the crash), honestly I don't think the code analysis mentioned in the description is right, given the client doesn't crash on a timeout (because unlike server, it doesn't have a watchdog), and the analysis is suggesting that is the reason for the crash, when a crash can only be caused by an exception, not an infinite loop, so I think some method call must be causing the crash instead. Plus their suggested fix changes behaviour of logging to "the process standard output", which I don't think even is System.out
(though I don't know for sure), so errors in there may not even get to the log. Personally I don't really think it's worth fixing it, sounds like a very edge-case very-specific-configuration-only rare crash.
Thanks again!
The title is wrong. It never crashes. If you read the What actually happened was...:
section you can see it hung and Linux brought up the option to force quit because it stopped responding. I've implemented a fix locally based on MC-200083's code analysis (duplicate report) and there is no lag (ProcessBuilder is non-blocking) and it didn't hang. First I will test what will happen if I open something and it crashes.
Note: standard output is System.out I believe
Edit: can confirm, redirect.inherit does the trick and I can see things being printed in the console
Also about the testing, let me recall what happened. (might be incorrect, just from memory)
- Test on Fabric - all good
- Test on Forge - bad. crash.
- Fix the forge crash
- Test on Forge - all good
I then forgot to check if the fix didn't break fabric
With this test I'm doing right now I got 2 logs into my normal minecraft output (appears to be System.err) from opening up a file that doesn't exist. Seems like some internal stuff but logs nonetheless
kf.service.services: KApplicationTrader: mimeType "x-scheme-handler/file" not found
kf.notifications: Audio notification requested, but sound file from notifyrc file was not found, aborting audio notification
And a UI popup
Standard output yes, but standard output of the process ("same as the current process") sounds like it's the one that's assigned by the OS (that's why I didn't edit the "main" open method in my mod, there was no way to redirect it to a logger like the game did). The standard output from a process (general not Java) is usually the console if launched in one, or nothing if not AFAIK.
I'll test this one day.