Multiple accounts / Settings per project

[reda-alaoui]

Fix #159

[uwolfer]

Wow, this is a huge change! Thanks for your work. I did not have the time to look into the code yet, but I've some questions:

1. Have you implemented it as proposed here?
2. How does the migration path look like for existing users?
3. What things have you tested so far?

If you do not mind, you could try to reduce the size of this change a little bit by removing all whitespace / indention only changes.

[reda-alaoui]

1. Have you implemented it as proposed here?

Almost. In addition to the aforementioned comment, I moved settings "CloneBaseUrl" to project settings level.

However, I didn't touch the ui. I thought the change was big enough to warrant a subsequent PR to add the UI hint.

2. How does the migration path look like for existing users?

For the existing users, it will be transparent.

I integrated a versioning system easing the settings migration process. The same system can be reused for future settings management modifications.

3. What things have you tested so far?

I tested the authentication, the code reviews grid listing and the settings migration.

[reda-alaoui]

Whitespaces/indentation changes were removed

[uwolfer]

Did a short test and works fine so far - great work!

Just wondering if we probably should make all settings project specific... the header in the settings page shows "For current project" anyway (independent of your change) and some settings like "Push to Gerrit by default" makes only sense for projects which have Gerrit enabled. What do you think?

[reda-alaoui]

I tried to do that at first. But I came to the conclusion that settings like the ones adding grid columns should be global. Most of the time, you want to toggle the grid columns for all projects.

IMO, « pushToGerritByDefault » is the only one left that should be moved. I want to do this in another pull request because the difficulty with this one is how we pull the settings from the main classloader using javassist.

Réda Housni Alaoui

Le 28 avr. 2020 à 22:30, Urs Wolfer [email protected] a écrit :

 Did a short test and works fine so far - great work!

Just wondering if we probably should make all settings project specific... the header in the settings page shows "For current project" anyway (independent of your change) and some settings like "Push to Gerrit by default" makes only sense for projects which have Gerrit enabled. What do you think?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[reda-alaoui]

I tried to do that at first. But I came to the conclusion that settings like the ones adding grid columns should be global. Most of the time, you want to toggle the grid columns for all projects.

Maybe I haven't been clear enough in this part. Suppose all settings are project level settings. Let be multiple projects. In this situation, if one day I decide to display the change number column, I will have to change the setting on each project.

[reda-alaoui]

We could also consider that all settings should be project level because IntelliJ encourages it. From this point of view, we can consider that the poor ergonomics is IntelliJ's fault, not ours. If people were to complain about that, we could redirect them to this kind of feature request or comment.

[uwolfer]

We could also consider that all settings should be project level because IntelliJ encourages it

I do not have a strong preference here. Both approaches have their own downsides. I think we can go with the approach which keeps the code simpler. It's up to you. :)

Will do more testing ASAP.

[reda-alaoui]

In this case, I prefer to keep it simple for this PR ;) Once this is merged, I will move pushToGerritByDefault to the project level.

[uwolfer]

@reda-alaoui: Sorry for my late feedback. I've just tried to test it with a recent IntelliJ version against some production tests, but failed to do so. I've manged to merge your PR into intellij2016.2 branch, but got some runtime errors related to saving settings.

Have you developed this change only against intellij14 branch or also against a recent version of it? You did everything as documented (develop PRs against intellij14), but in this case it might make sense to develop it only against the most recent version because it is such a heavy change and also IntelliJ related settings infrastructure changed after IntelliJ 14. Would it be possible for you to rebase this PR on intellij2016.2 branch and try to get it running there (incl. using an already existing setup / Gerrit plugin settings XML)?

[reda-alaoui]

@uwolfer ok I will try that asap

[reda-alaoui]

@uwolfer I locally rebased on intellij2016.2 . I have no error while saving the settings. Could you tell me what you do exactly so that I try to reproduce? The error stacktrace would be handful too.

[uwolfer]

I'm getting errors after playing around in the plugin settings (but it could be possible that I've failed resolving some merge conflicts):

2020-05-13 21:12:31,855 [ 184011] WARN - mponents.impl.stores.StoreUtil - Save settings failed java.lang.RuntimeException: java.lang.Exception: Cannot get GerritSettings component state at com.intellij.util.ExceptionUtil.rethrow(ExceptionUtil.java:116) at com.intellij.util.lang.CompoundRuntimeException.throwIfNotEmpty(CompoundRuntimeException.java:106) at com.intellij.configurationStore.SaveResult.throwIfErrored(SaveResult.kt:59) at com.intellij.configurationStore.ComponentStoreImpl.save$suspendImpl(ComponentStoreImpl.kt:155) at com.intellij.configurationStore.ComponentStoreImpl$save$1.invokeSuspend(ComponentStoreImpl.kt) at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33) at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56) at kotlinx.coroutines.EventLoopImplBase.processNextEvent(EventLoop.common.kt:271) at kotlinx.coroutines.BlockingCoroutine.joinBlocking(Builders.kt:79) at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking(Builders.kt:54) at kotlinx.coroutines.BuildersKt.runBlocking(Unknown Source) at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking$default(Builders.kt:36) at kotlinx.coroutines.BuildersKt.runBlocking$default(Unknown Source) at com.intellij.configurationStore.SaveAndSyncHandlerImpl.processTasks(SaveAndSyncHandlerImpl.kt:83) at com.intellij.configurationStore.SaveAndSyncHandlerImpl.access$processTasks(SaveAndSyncHandlerImpl.kt:47) at com.intellij.configurationStore.SaveAndSyncHandlerImpl$saveAlarm$1.invoke(SaveAndSyncHandlerImpl.kt:59) at com.intellij.configurationStore.SaveAndSyncHandlerImpl$saveAlarm$1.invoke(SaveAndSyncHandlerImpl.kt:47) at com.intellij.util.SingleAlarmKt$sam$java_lang_Runnable$0.run(SingleAlarm.kt) at com.intellij.util.concurrency.QueueProcessor.runSafely(QueueProcessor.java:232) at com.intellij.util.Alarm$Request.runSafely(Alarm.java:367) at com.intellij.util.Alarm$Request.run(Alarm.java:357) at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515) at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) at com.intellij.util.concurrency.SchedulingWrapper$MyScheduledFutureTask.run(SchedulingWrapper.java:220) at com.intellij.util.concurrency.BoundedTaskExecutor.doRun(BoundedTaskExecutor.java:223) at com.intellij.util.concurrency.BoundedTaskExecutor.access$200(BoundedTaskExecutor.java:30) at com.intellij.util.concurrency.BoundedTaskExecutor$1.execute(BoundedTaskExecutor.java:202) at com.intellij.util.ConcurrencyUtil.runUnderThreadName(ConcurrencyUtil.java:210) at com.intellij.util.concurrency.BoundedTaskExecutor$1.run(BoundedTaskExecutor.java:191) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) at java.base/java.lang.Thread.run(Thread.java:834) Caused by: java.lang.Exception: Cannot get GerritSettings component state at com.intellij.configurationStore.ComponentStoreImpl.commitComponents$intellij_platform_configurationStore_impl(ComponentStoreImpl.kt:222) at com.intellij.configurationStore.ComponentStoreWithExtraComponents.commitComponents$intellij_platform_configurationStore_impl(ComponentStoreWithExtraComponents.kt:90) at com.intellij.configurationStore.ComponentStoreImpl$commitComponentsOnEdt$$inlined$withEdtContext$intellij_platform_configurationStore_impl$1.invokeSuspend(ComponentStoreImpl.kt:695) at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33) at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56) at com.intellij.configurationStore.EdtPoolDispatcherManager.processQueue(EdtPoolDispatcher.kt:54) at com.intellij.configurationStore.EdtPoolDispatcherManager.access$processQueue(EdtPoolDispatcher.kt:18) at com.intellij.configurationStore.EdtPoolDispatcherManager$scheduleFlush$1.invoke(EdtPoolDispatcher.kt:32) at com.intellij.configurationStore.EdtPoolDispatcherManager$scheduleFlush$1.invoke(EdtPoolDispatcher.kt:18) at com.intellij.configurationStore.EdtPoolDispatcherKt$sam$java_lang_Runnable$0.run(EdtPoolDispatcher.kt) at com.intellij.openapi.application.TransactionGuardImpl$2.run(TransactionGuardImpl.java:205) at com.intellij.openapi.application.impl.ApplicationImpl.runIntendedWriteActionOnCurrentThread(ApplicationImpl.java:831) at com.intellij.openapi.application.impl.ApplicationImpl.lambda$invokeLater$4(ApplicationImpl.java:310) at com.intellij.openapi.application.impl.FlushQueue.doRun(FlushQueue.java:80) at com.intellij.openapi.application.impl.FlushQueue.runNextEvent(FlushQueue.java:128) at com.intellij.openapi.application.impl.FlushQueue.flushNow(FlushQueue.java:46) at com.intellij.openapi.application.impl.FlushQueue$FlushNow.run(FlushQueue.java:184) at java.desktop/java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:313) at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:776) at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:727) at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:721) at java.base/java.security.AccessController.doPrivileged(Native Method) at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:85) at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:746) at com.intellij.ide.IdeEventQueue.defaultDispatchEvent(IdeEventQueue.java:974) at com.intellij.ide.IdeEventQueue._dispatchEvent(IdeEventQueue.java:847) at com.intellij.ide.IdeEventQueue.lambda$null$8(IdeEventQueue.java:449) at com.intellij.openapi.progress.impl.CoreProgressManager.computePrioritized(CoreProgressManager.java:739) at com.intellij.ide.IdeEventQueue.lambda$dispatchEvent$9(IdeEventQueue.java:448) at com.intellij.openapi.application.impl.ApplicationImpl.runIntendedWriteActionOnCurrentThread(ApplicationImpl.java:831) at com.intellij.ide.IdeEventQueue.dispatchEvent(IdeEventQueue.java:496) at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203) at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124) at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:113) at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:109) at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101) at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:90) Caused by: com.intellij.util.IncorrectOperationException: Can't change immutable element: class org.jdom.ImmutableElement. To obtain mutable Element call .clone() at org.jdom.ImmutableElement.immutableError(ImmutableElement.java:317) at org.jdom.ImmutableElement.setAttribute(ImmutableElement.java:433) at com.urswolfer.intellij.plugin.gerrit.settings.GerritSettings.getState(GerritSettings.java:72) at com.urswolfer.intellij.plugin.gerrit.settings.GerritSettings.getState(GerritSettings.java:40) at com.intellij.configurationStore.ComponentStoreImpl.commitComponent(ComponentStoreImpl.kt:308) at com.intellij.configurationStore.ComponentStoreImpl.commitComponents$intellij_platform_configurationStore_impl(ComponentStoreImpl.kt:218) ... 36 more

[uwolfer]

@reda-alaoui: If you do not find time to look into my exception, you could also start a new PR which is based on the intellij16.2 branch - then I can do tests based on this.

[reda-alaoui]

@uwolfer Ok I will. Sorry I didn't have much time lately.

[reda-alaoui]

@uwolfer I rebased the PR on intellij16.2

[leoluk]

Hey, any progress on this? This would be super useful to have, I regularly interact with different Gerrit servers.

Is there a donation pool or bounty platform to contribute to? Glad to contribute $100 to this effort.

[uwolfer]

@leoluk: Unfortunately there is no progress from my side. I'm lacking a bit of time, but it would be really great if we could get that landed. The biggest help in this would be if somebody could test the whole change implemented by @reda-alaoui, also regarding backwards compatibility.

[leoluk]

Happy to help with that. What combinations need testing for backwards compatibility?

[uwolfer]

@leoluk: Some constellations I'd like to have tested:

1. complete new IntelliJ project setup (including multiple Gerrit instances setup)
2. existing single project IntelliJ support, setup with current version of this plugin, then migrated to the new version with multi project support
3. existing multi project IntelliJ support, setup with current version of this plugin, then migrated to the new version with multi project support

You can find a guide how to setup a development environment for this plugin here. You support is highly appreciated!

[reda-alaoui]

I think this PR is officialy dead :)