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

/mv clone command #780

Closed
wants to merge 6 commits into from
Closed

/mv clone command #780

wants to merge 6 commits into from

Conversation

mathphreak
Copy link
Contributor

Adding /mv clone per #779

@mathphreak mathphreak mentioned this pull request Jul 19, 2012
@fernferret
Copy link
Member

Brief read is looking good, will test on or before this weekend and pull, thanks!

@mathphreak
Copy link
Contributor Author

My code doesn't copy over world properties set with /mv modify, and I can't figure out how to copy those in bulk, but it shouldn't be too hard.

@fernferret
Copy link
Member

Ideally it would just make an in memory copy of one of the MVWorld objects, then save that object to disk (yaml).

EDIT: In addition to what you're doing with the filesystem.

@dumptruckman
Copy link
Member

Before we pull this I'd like to see the file copy method moved from Multiverse-Adventure's FileUtil to cores.

@dumptruckman
Copy link
Member

And here it is: 085c7a1
Please work with the new code base. Make sure to "git pull -r" to replay your commits on top of the new codebase and avoid a nasty merge message.

@dumptruckman
Copy link
Member

Hmm, and it would appear if you use our method you'll have to delete the uid.dat afterwards. I still think that would be the better course of action.

@fernferret
Copy link
Member

Maybe modifying the utils method to allow deletion of the uid as a parameter?

@mathphreak
Copy link
Contributor Author

Now using FileUtils.copyFolder().

}

private void deleteUID(File worldFolder) throws IOException {
File uidFile = new File(worldFolder, "uid.dat");
Copy link
Member

Choose a reason for hiding this comment

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

Wrong indentation

@fernferret
Copy link
Member

Sorry @mathphreak had a busy weekend (read: slacker playing the Guild Wars 2 beta...) I'll see about looking at it sometime this week, won't be tonight. It looks as if @main-- and @dumptruckman have already given their input, and you've fixed all they've asked.

@dumptruckman
Copy link
Member

So... After more careful review I've decided that the world copy will have to be asynchronous or else copying a huge world folder will lag out your server. As a side effect of being asynchronous, you would have to kick everyone out of the fromWorld and unload it BEFORE you copy it. Since this is kind of a "what???" procedure, it would probably be best to require and /mvconfirm first that warns them what is going to happen.

@mathphreak
Copy link
Contributor Author

As a side effect of requiring /mvconfirm, I've had to move the cloning logic from CloneCommand to WorldManager. But that's probably a better place for it anyways.

@fernferret
Copy link
Member

Nice move @mathphreak I agree entirely. We try to keep a lot of the logic out of the actual command class as it keeps those cleaner.

I'll look at it tonight or tomorrow; test and pull.

@fernferret
Copy link
Member

Also @main-- I appreciate your critique of tabbing, but I'll just have IntelliJ zap all the imported files :P

@fernferret
Copy link
Member

What's this @mathphreak? I'm actually looking at this PR now?!?! Blasphemy! Seriously. Sorry for the delay.

  • Slight issue with the clone command help formatting,
  • Added alias /mvcl and /mv cl for the command.

More to come as I continue to test and merge.

@fernferret
Copy link
Member

Overall, works great!

@ghost ghost assigned fernferret Aug 4, 2012
@fernferret
Copy link
Member

Merged in f574681. Closes #779. Thanks!

@fernferret fernferret closed this Aug 4, 2012
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.

4 participants