PlanarAlly
PlanarAlly copied to clipboard
Kruptein
Administrative features and some DM tools
I'll do a more in-depth review later, but some quick remarks.
Ideally you would split up the different changes in multiple PR's. Smaller PRs are easier to review but also make it so that your potential changes can get merged faster whereas they would now have to wait on the entire list of changes to be ok. You made a separate PR for the UI icons which is nice, but you included those commits in this PR as well, which you preferably wouldn't do. (There is no need to try to fix this now, I'll do the review on this list, it's just to let you know so that you can improve upon this in the future!)
As the UI icon change has it's own PR I'll discuss those changes in that PR.
Now onto the specifics
the "focus everyone' feature is more or less already implemented in the right click menu "bring players" action. Is there a specific reason why you dislike that or add this extra feature? Also I would suggest to move it from the DM settings to the right click menu as I can imagine you would want to use this somewhat regularly and opening the DM settings everytime can become cumbersome in that regard.
The locations delete option is something that is very needed, but I would like to add this to the actual location panel at the top along with some other changes to allow for reordering etc. This is however something for the future, so your current suggestion may be a god solution until I get the time to redesign the locations bar.
You also mention somewhere "Be aware that only empty locations can be removed currently.". Why wouldn't we allow the removal of locations with shapes ?
but you included those commits in this PR as well
That's my bad, I somehow branched this one from a wrong point, I'll be more careful in future
the "focus everyone' feature
My players needed this feature badly and I somehow missed it in right click menu. We don't need it too too often, but, if other parties do, you're right and DM options is not a good place for it.
The locations delete option
We just had too many locations in our session, so I added this thing to the first place I saw.
Why wouldn't we allow the removal of locations with shapes ?
A database error just happen to be risen when I try to remove a location with foreign objects and I didn't did anything to the model for those objects to be unlinked or removed on deletion. I also didn't found a way to return some user-readable error for this case, so I just put a warning for now.
I also strongly believe that we need something like an IRC channel for developers to discuss minor stuff like where to put what button. I'd like to suggest freenode.net service for this.
A database error just happen to be risen
Right, I would like to have this work for all locations though, I'll take a look at what needs to be done database wise to make this work.
I also strongly believe that we need something like an IRC channel
I've used IRC a lot in the past, but the problem I feel for this kind of stuff where there is going to be little users online, is that you need to be either always online or check the logs to see if something was said. I would rather use something like discord or slack then.
I somehow missed it in right click menu.
Does this mean you can remove that part of this PR?
Does this mean you can remove that part of this PR?
If only I knew how to do it =)
I'll have to research a way.
You simply remove the changes from your branch, and make a new commit! If you push the commit it will automatically update the PR. :)
remove the changes from your branch
I am stuck at this point, researching
Or should I just make more changes that'll undo these changes?
Yes this should just be the same procedure as how you added the changes in the first place. While you're at it, immediately remove the icon changes as well as they are discussed in the other PR.
It's been a while so I'm closing this, I think most of the things in this PR are either already implemented at this point or no longer relevant.