search

uwolfer / gerrit-intellij-plugin

Gerrit Code Review Tool Integration for the IntelliJ Platform
Apache License 2.0
495 stars 103 forks source link

Multiple accounts / Settings per project #382

Closed reda-alaoui closed 2 years ago

reda-alaoui commented 4 years ago

Fix #159

uwolfer commented 4 years ago

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 commented 4 years ago
  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.

  1. 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.

  1. What things have you tested so far?

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

reda-alaoui commented 4 years ago

Whitespaces/indentation changes were removed

uwolfer commented 4 years ago

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 commented 4 years ago

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 notifications@github.com 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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@uwolfer ok I will try that asap

reda-alaoui commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

reda-alaoui commented 4 years ago

@uwolfer I rebased the PR on intellij16.2

leoluk commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

uwolfer commented 4 years ago

@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 commented 2 years ago

I think this PR is officialy dead :)