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

Improve Matrix class #5887

Merged
merged 17 commits into from
May 16, 2022
Merged

Improve Matrix class #5887

merged 17 commits into from
May 16, 2022

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented May 2, 2022

Small change in the Matrix class: deprecated methods have been removed and the constructor is now public. Also the fun workerFactory() has been renamed to getWorkerFactory()

Documentation has been improved to.

Knit has been set up, also on the CI. For now we are not doing a big usage of it, but can be more useful in the future.

Also generated documentation header will have working link:

image

(see previous version without links in #5854)

@bmarty bmarty requested review from a team, Claire1817 and ariskotsomitopoulos and removed request for a team May 2, 2022 10:04
@bmarty bmarty marked this pull request as draft May 2, 2022 10:04
@github-actions
Copy link

github-actions bot commented May 2, 2022

Unit Test Results

122 files  122 suites   2m 32s ⏱️
205 tests 205 ✔️ 0 💤 0
690 runs  690 ✔️ 0 💤 0

Results for commit 0ed647d.

♻️ This comment has been updated with latest results.

@bmarty bmarty force-pushed the feature/bma/Matrix branch from 0df0dae to 03f35f6 Compare May 4, 2022 12:22
@bmarty bmarty marked this pull request as ready for review May 4, 2022 15:23
fun getUserAgent() = userAgentHolder.userAgent

/**
* Return the AuthenticationService
*/
fun authenticationService() = authenticationService
Copy link
Contributor

Choose a reason for hiding this comment

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

not for this PR, we have a mix of get${object} and ${object}, is it something we want to align on in the public api?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we have a mix, the (un-official!) rule so far is to not use the get prefix for the SDK services, and I think this is the case for all services, to avoid having long name for the methods.

Happy to discuss this more, and/or to change the rule.

I have renamed workerFactory() because this is not a SDK service.

@bmarty bmarty force-pushed the feature/bma/Matrix branch from 997e51a to 0ed647d Compare May 16, 2022 10:25
@bmarty
Copy link
Member Author

bmarty commented May 16, 2022

thanks for the review. I have force pushed to fix merge conflict, also added a last commit to fix Kdoc issue.

@bmarty bmarty enabled auto-merge May 16, 2022 10:26
@bmarty bmarty merged commit 955f366 into develop May 16, 2022
@bmarty bmarty deleted the feature/bma/Matrix branch May 16, 2022 11:08
@github-actions
Copy link

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    = passed=7 failures=13 errors=0 skipped=3
  • [org.matrix.android.sdk.account]
    = passed=1 failures=2 errors=0 skipped=2
  • [org.matrix.android.sdk.internal]
    = passed=5 failures=1 errors=0 skipped=0
  • [org.matrix.android.sdk.ordering]
    = passed=16 failures=0 errors=0 skipped=0
  • [org.matrix.android.sdk.PermalinkParserTest]
    = passed=2 failures=0 errors=0 skipped=0

@michaelkaye
Copy link
Contributor

michaelkaye commented May 16, 2022

https://github.com/vector-im/element-android/runs/6450886478?check_suite_focus=true#step:8:392

I think this has caused a regression in the integration tests (this also happened to me locally) - looks like we've changed how we instantiate the TestMatrix, and the android WorkManagerImpl is complaining.

 org.matrix.android.sdk.session.search.SearchMessagesTest > sendTextMessageAndSearchPartOfItUsingSession[test(AVD) - 9] FAILED 
	java.lang.IllegalStateException: WorkManager is already initialized.  Did you try to initialize it manually without disabling WorkManagerInitializer? See WorkManager#initialize(Context, Configuration) or the class level Javadoc for more information.
	at androidx.work.impl.WorkManagerImpl.initialize(WorkManagerImpl.java:185)

org.matrix.android.sdk.session.space.SpaceCreationTest > createSimplePublicSpace[test(AVD) - 9] FAILED 
	java.lang.IllegalStateException: WorkManager is already initialized.  Did you try to initialize it manually without disabling WorkManagerInitializer? See WorkManager#initialize(Context, Configuration) or the class level Javadoc for more information.
	at androidx.work.impl.WorkManagerImpl.initialize(WorkManagerImpl.java:185)

@michaelkaye
Copy link
Contributor

michaelkaye commented May 16, 2022

        val delegate = WorkManagerImpl(
                context,
                configuration,
                WorkManagerTaskExecutor(configuration.taskExecutor)
        )
        WorkManagerImpl.setDelegate(delegate)

Might work (so using the unit test delegate thing to handle it) to avoid repeated initialization BUT, it's still a global and I think this means we need to prevent tests running in parallel otherwise race conditions will happen.

(trying this locally to see how it fares)

@bmarty
Copy link
Member Author

bmarty commented May 16, 2022

Thanks for your investigation

piersonleo pushed a commit to piersonleo/element-android that referenced this pull request Jun 7, 2022
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