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

bitECS: Fix move permissions #6253

Merged
merged 4 commits into from
Sep 28, 2023
Merged

bitECS: Fix move permissions #6253

merged 4 commits into from
Sep 28, 2023

Conversation

keianhzo
Copy link
Contributor

Fixes #6250

This PR fixes the moving permission for media.

@keianhzo keianhzo requested a review from takahirox September 13, 2023 10:20
@takahirox takahirox added P1 Address as quickly as possible new-loader labels Sep 14, 2023
@takahirox takahirox changed the title Fix move permissions bitECS: Fix move permissions Sep 14, 2023
(!isPinned || window.APP.hubChannel.can("pin_objects")) &&
(!isPen || window.APP.hubChannel.can("spawn_drawing")))
);
export function canMove(elOrEid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As I mentioned somewhere else, I don't prefer elOrEid style because I think it's an error prone. Explicitly separating functions would be clearer and simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The one thing I like about the elOrEid approach is that avoids branching in a lot of places when the function is used across the codebase. In this case canMove is only used in a couple of places so I think it's fine to duplicate the function.

@keianhzo
Copy link
Contributor Author

@takahirox I've separated the functions and also re-enable a commented canMove in app-aware-touchscreen.js.

return (
hasComponent(APP.world, HoldableButton, eid) ||
(APP.hubChannel.can("spawn_and_move_media") && (!isPinned(eid) || APP.hubChannel.can("pin_objects")))
);
Copy link
Contributor

@takahirox takahirox Sep 27, 2023

Choose a reason for hiding this comment

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

Review note: Honestly I'm not really familiar well enough with permission check but it looks based on utils/permission-utils.js so I want to approve for now.

@takahirox
Copy link
Contributor

re-enable a commented canMove in app-aware-touchscreen.js.

Is this related to #6250?

Copy link
Contributor

@takahirox takahirox left a comment

Choose a reason for hiding this comment

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

Approves once #6253 (comment) will be clear

@keianhzo
Copy link
Contributor Author

@takahirox yes, it's related to that, otherwise you could move pinned objects.

@keianhzo keianhzo requested a review from takahirox September 27, 2023 16:04
@takahirox
Copy link
Contributor

OK, good to go

@keianhzo keianhzo merged commit c01d792 into master Sep 28, 2023
9 of 11 checks passed
@keianhzo keianhzo deleted the bitecs-fix-move-permissions branch September 28, 2023 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-loader P1 Address as quickly as possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bitECS: "Create and move objects" permission doesn't work correctly
2 participants