Closed toad-dev closed 3 years ago
SquidDev
commented
3 years ago It's probably worth switching to use the COPY_TO_CLIPBOARD click action. Old versions of MC didn't have this (hence the presence of CommandCopy), but it can be dropped now.
See https://github.com/SquidDev-CC/CC-Tweaked/commit/9f7cc00fcbbc7516def382168c6ef5e5d6ed2c06#diff-afb28f2a81e6c9c6625288974f6971e60f62bdaceec73070d6b87476feb6c721R94-R101 for what I changed in Forge.
toad-dev
commented
3 years ago It's probably worth switching to use the
COPY_TO_CLIPBOARDclick action. Old versions of MC didn't have this (hence the presence ofCommandCopy), but it can be dropped now.See SquidDev-CC/CC-Tweaked@9f7cc00#diff-afb28f2a81e6c9c6625288974f6971e60f62bdaceec73070d6b87476feb6c721R94-R101 for what I changed in Forge.
Merging this commit fixed a good number of issues with the /computercraft command on fabric. I'm having an issue still that I'd like to pick your brain about: /computercraft view initially shows a blank terminal. I have to update it by pressing a key for the contents to pop in. Weirdly this doesn't happen if I interact with computer before running view on it. This bug doesn't happen on forge and I can't track down an obvious difference to explain why.
SquidDev
commented
3 years ago Well, took me a while to work out how all the terminal synchronisation code plugs together (which is awkward, as I wrote it).
Basically, CC: Tweaked listens to when IContainerComputer containers are opened and sends the terminal state to the player. This basically means all the terminal GUIs work automatically.
However, the Fabric port sends the terminal state at the point where the GUI is initially opened. I know that my own Fabric version did this too (https://github.com/SquidDev-CC/CC-Tweaked/blob/45e84e1edec2866e7551031556595e6dc0375864/src/main/java/dan200/computercraft/shared/network/Containers.java#L41), but can't really remember the history of this. Anyway, beside the point...
The fix here is either to call sendTerminalState (like currently done in TileComputerBase and ItemPocketComputer already), or add a hook to listen to container opens. I can't remember if Fabric provides an existing one for this, so an explicit sendTerminalState may be easier.
toad-dev
commented
3 years ago Thank you! Adding a call to sendTerminalState works great. Ideally I'd like to mimic the forge event hook as I think making this fabric port mirror CC: Tweaked as much as possible would make merging less error prone. (Though the code style and mappings are the more immediate problem with merges right now... not to mention the even more insidious changes in method order.)
I'll have to do some more sifting through the fabric api and maybe weigh whether a custom event hook is worth the risk.
@Merith-TK I don't know how to mark this as draft from the app, but hold off on merging while I work on it a bit more.
Merith-TK
commented
3 years ago I actually have a question about the /computercraft view command @SquidDev
Which also a bit scuffed in order. (0, 1, 10, 12, 2, 20,25)
On fabric it shows the numbers in order of Activation, Rather than the computer ID itself, is this normal for the command on CC:T?
SquidDev
commented
3 years ago On fabric it shows the numbers in order of Activation, Rather than the computer ID itself, is this normal for the command on CC:T?
Are you referring to auto-complete here? The ordering is handled by Mojang (which I guess is just lexicographical), but the fact that it's order of activation (or rather, the "instance ID" in ServerComputerRegistry) is intentional. Computer IDs are not guaranteed to be unique, but these are.
It's possible to refer to a computer by its computer ID by prefixing it with # (so #12 is a computer with id 12). See ComputersArgumentType (this bit and onwards):
toad-dev
commented
3 years ago Okay I just added the one call to sendTerminalState. Fabric doesn't provide a hook quite as nice as what's used in CC: Tweaked and, as for a mixin, well I'm just too confused by minecraft's gui code. And it's in no way worth it for one line anyway.
@Merith-TK this PR should be ready to test. Sorry about the confusing diffs. I took the opportunity while merging to make a few touched files consistent in style with tweaked. I think in the future we should just restyle the whole repo with same code style rules as tweaked... and throw the m_'s in the bin too.
Merith-TK
commented
3 years ago Let me know when this is complete and mergable
On Mon, May 31, 2021, 5:08 PM ToadDev @.***> wrote:
Okay I just added the one call to sendTerminalState. Fabric doesn't provide a hook quite as nice as what's used in CC: Tweaked and, as for a mixin, well I'm just too confused by minecraft's gui code. And it's in no way worth it for one line anyway.
@Merith-TK https://github.com/Merith-TK this PR should be ready to test. Sorry about the confusing diffs. I took the opportunity while merging to make a few touched files consistent in style with tweaked. I think in the future we should just restyle the whole repo with same code style rules as tweaked... and throw the m_'s in the bin too.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Merith-TK/cc-restitched/pull/43#issuecomment-851723727, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACPQOXSOTKTIJTCGDAENP3LTQQQGPANCNFSM45ZRZRNA .
toad-dev
commented
3 years ago Let me know when this is complete and mergable
It's good to go.
~
CommandCopywas annotated as a client only class, but it needs to be present on the server for this feature to work.~Per @SquidDev's suggestion, I ported the commit from the forge repo that changes how this is handled. While merging the changes I was able to fix most (🤞) of what was wrong with the /computercraft command too. So this PR really contains two fixes:
I hope jumping ahead and cherry picking this commit doesn't break Jummit's process for keeping track of ported commits.