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

Added functionality for having multiple packets open at the same time. #13

Merged
merged 3 commits into from
Jul 30, 2020
Merged

Added functionality for having multiple packets open at the same time. #13

merged 3 commits into from
Jul 30, 2020

Conversation

realDragonium
Copy link
Contributor

Hey,

I was looking for something like this, but I thought having multiple packets open at the same time works a bit better when you want to compare packets with each other.

And my IDE found it a good idea if some dependencies got added/were updated. 😅

Copy link
Owner

@wvffle wvffle left a comment

Choose a reason for hiding this comment

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

Two minor issues but thanks for PR!

It still has a couple of bugs as I left it in the middle of rewriting it. I have no free time right now to fix them so if you're willing to fix them, it'd be awesome!

src/App.vue Outdated
@@ -381,7 +380,7 @@
}
},
listening: false,
id_open: null,
open_ids: [],
Copy link
Owner

Choose a reason for hiding this comment

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

Use a new Set() rather than an array

}

window.packet = packet
Copy link
Owner

Choose a reason for hiding this comment

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

That was for quick debugging in console. I'd rather not resign from this behavior. Having an ability to manipulate over opened packet (packets in case of this PR) in console is a must have in my case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, so that's what it was for. I have no problem with it so I will put it back. How can you use this?

@wvffle wvffle mentioned this pull request Jul 26, 2020
@realDragonium
Copy link
Contributor Author

Let me know what those bugs are and I will see what I can do about them!

I have also been working on making it work for online-mode servers. Did this also included the scope of your project?

@wvffle
Copy link
Owner

wvffle commented Jul 30, 2020

Let me know what those bugs are and I will see what I can do about them!

I've highlighted them in the review

I have also been working on making it work for online-mode servers. Did this also included the scope of your project?

Yup, right now localhost 1.12.2 is hardcoded as far as I remember. You should be able to change the server in the client settings

There is now an option for toggling or you want to open multiple packets at the same time.
Rename `open_ids` to `open_packets`. Also have tried to make a `Set` of it, but it didnt open them when you clicked on them.
@wvffle
Copy link
Owner

wvffle commented Jul 30, 2020

idk if it's still there but you can trace server.host variable in index.js of 8153464. There's some stuff that's responsible for changing the server

@realDragonium
Copy link
Contributor Author

realDragonium commented Jul 30, 2020

Use a new Set() rather than an array

It doesnt work anymore when its a Set so it will have to stay as an array for now.

@wvffle
Copy link
Owner

wvffle commented Jul 30, 2020

Lets stick with an array then. Can you expose open_packets to window and I'll merge it?

Edit: I've seen that you exported packet. That's ok for now

@realDragonium
Copy link
Contributor Author

realDragonium commented Jul 30, 2020

window.packet = this.open_packets

like this? or should window.open_packet be better?

@wvffle wvffle merged commit 53105a9 into wvffle:master Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants