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.

Sign up for GitHub

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

Jump to bottom

Check power level before starting live sharing location #7808

Closed
wants to merge 7 commits into from
Closed

Check power level before starting live sharing location #7808

wants to merge 7 commits into from

Conversation

NicolasBuquet
Copy link

Pull Request Checklist

  • I read the contributing guide
  • UI change has been tested on both light and dark themes, in portrait and landscape orientations and on iPhone and iPad simulators
  • Accessibility has been taken into account.
  • Pull request is based on the develop branch
  • Pull request contains a changelog file in ./changelog.d
  • You've made a self review of your PR
  • Pull request includes screenshots or videos of UI changes
  • Pull request includes a sign off

@NicolasBuquet
Copy link
Author

@pixlwave A small change to enhance Element iOS ;-)

We made this change on Tchap.

@NicolasBuquet NicolasBuquet mentioned this pull request Jun 13, 2024
Open
@NicolasBuquet NicolasBuquet force-pushed the nbuquet/check-power-level-before-starting-live-sharing-location- branch from bb22a72 to ca76c57 Compare June 13, 2024 09:39
pixlwave
pixlwave reviewed Jun 20, 2024
View reviewed changes
Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

Thanks, this looks sensible to me. Have made a couple of suggestions for it :)

...UI/Modules/LocationSharing/StartLocationSharing/Coordinator/LocationSharingCoordinator.swift Outdated Show resolved Hide resolved
...UI/Modules/LocationSharing/StartLocationSharing/Coordinator/LocationSharingCoordinator.swift Outdated
Comment on lines 175 to 176
// Should call `minimumPowerLevelForSendingStateEvent(_ eventType: MXEventType) -> Int` from `MatrixSDK:MXRoomPowerLevels.swift`
// but can't because it is inaccessible due to 'internal' protection level
Copy link
Member

Choose a reason for hiding this comment

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

That seems purely like an oversight when refining the objc for Swift. Would you mind making these 2 public and updating the PR based upon that:

https://github.com/matrix-org/matrix-ios-sdk/blob/develop/MatrixSDK/Contrib/Swift/JSONModels/MXRoomPowerLevels.swift

It would give us a much neater implementation here.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

Need to compile against updated matrix-iso-sdk lib (matrix-org/matrix-ios-sdk#1869)

Copy link
Member

Choose a reason for hiding this comment

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

Perfect, it's merged so feel free to update the submodule and push 👍

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, what do you mean by 'update the submodule and push" ?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, the SDK as a pod was deprecated recently, and EI switched to consuming it as a git submodule. Don't worry, I've opened #7819 with the update, so once that is merged just rebase on develop and CI should be happy.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, thank you @pixlwave.

I followed the matrix-sdk pod deprecation, but didn't follow how to integrate matrix-sdk update into git-submodule.

I will study #7819 👍

Copy link
Member

Choose a reason for hiding this comment

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

It's merged.

@NicolasBuquet NicolasBuquet force-pushed the nbuquet/check-power-level-before-starting-live-sharing-location- branch from ca76c57 to b44685c Compare June 24, 2024 10:04
@NicolasBuquet
Copy link
Author

@giomfo Can you follow these please. Thanks 🙏

This was referenced Jun 24, 2024
Merged
Merged
@pixlwave
Copy link
Member

@NicolasBuquet Are you planning on coming back to this PR? We still need a rebase for CI to pass please.

@NicolasBuquet
Copy link
Author

@NicolasBuquet Are you planning on coming back to this PR? We still need a rebase for CI to pass please.

Hello @pixlwave
I finished on this PR. It's ok for me.
The CI is quite broken for me and I don't intend to fix it.
Thank you for your involvement.

@pixlwave
Copy link
Member

@NicolasBuquet If you rebase your branch (or merge develop into your branch if that's easier) that's all we need, CI will work and I can merge this PR for you :)

Nicolas Buquet added 2 commits August 14, 2024 16:26
Merge branch 'develop' into nbuquet/check-power-level-before-starting…
55951df 
…-live-sharing-location-
Merge branch 'nbuquet/check-power-level-before-starting-live-sharing-…
4d50157 
…location-' of github.com:NicolasBuquet/element-ios-tchap into nbuquet/check-power-level-before-starting-live-sharing-location-
@NicolasBuquet
Copy link
Author

@NicolasBuquet If you rebase your branch (or merge develop into your branch if that's easier) that's all we need, CI will work and I can merge this PR for you :)

Merge done @pixlwave

@pixlwave pixlwave mentioned this pull request Aug 14, 2024
Merged
@pixlwave
Copy link
Member

Replacing this PR with #7832 as #7778 seems to have slipped into here too.

@pixlwave pixlwave closed this Aug 14, 2024
@NicolasBuquet NicolasBuquet mentioned this pull request Sep 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pixlwave pixlwave pixlwave left review comments

Assignees

@pixlwave pixlwave

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NicolasBuquet @pixlwave