Community
Participate
Working Groups
Discovered when testing https://git.eclipse.org/r/#/c/150328/ : somehow the RepositoryNode allocated in the staging view ends up being referenced also from the tree in the repository viewer. The staging view assumes it's the sole owner of that node and clears it when it doesn't use it anymore. But the node may then still be in use in the repository view, which then has a RepositoryNode with a null repository, which breaks subsequent operations. This has never occurred until now as far as I know (I really wish we still had AERI, then I could search for relevant exceptions) but it's actually very easy to provoke: 1. Run Eclipse and prepare it: Java perspective; open repo view, add a few repos. Keep staging view closed. Quit. 2. Start Eclipse on that workspace. Comes up with Java perspective and repo view open, staging view closed. There's no repo selected in the repo view. 3. Go to Windows->Show-View->Others..., select the staging view. 4. Staging view comes up, shows "[No repository]". Check "follow selection" if not already checked. 6. Click on one of the repos (let's call is R) in the repo view. 7. Staging view shows status of repo R. Title shows the repo name (and branch state). 8. Close the staging view by clicking the "x" icon in its tab. 9. Mouse over to the repo view and try to expand the repo node for R (click on the triangle). 10. Ka-boom. Repo view is broken.
This is related to lightweight decorators. The DecoratorManager fires an event containing _all_ objects, from anywhere, whose decorations have arrived. When I examine the events gotten in the repo view, I even see two WorkingSets from the package explorer in there! The StagingView uses its own instance of RepositoryNode (called titleNode) and its own instance of RepositoryTreeNodeLabelProvider, which uses the lightweight decorator. But when that lightweight decorator gets the decoration, the DecoratorManager fires an event with this titleNode. All label providers using the DecoratorManager will get that event and propagate it, causing the viewers that use them to update their items for this RepositoryNode. So the repo view updates in response to a LabelProviderChangedEvent from the DecoratorManager, caused by a decoration scheduled for the label provider of the staging view. The repo view's tree viewer now gets a RepositoryNode from elsewhere that compares equal to a RepositoryNode already in the view. In AbstractTreeViewer.associate() the binding from the TreeItem to the RepositoryNode (and back) is then updated. That's how this node from the staging view ends up in the repo view tree! And then when the staging view clears its node, which it thought to be the sole owner of, we suddenly have a RepositoryNode in the repo view with a null repository. This is a problem with the general design of that repo view. Typically, input to a viewer is the model you want to show. In our case that would be the actual Repository. So when the label of a unique model object has changed, it's fine to notify all views (or their label providers) that the visual representation of that object needs to be updated, and it's even ok to put a different but equal instance in some particular tree. But in our case there's a separate layer, the RepositoryTreeNodes, which is a view-specific view model. Which is fine as long as this view model is used only in that view. But we use RepositoryTreeNodes elsewhere, too, and consider different instances really different even though they compare equal, and that breaks things now. Given that from a model-view point of view the operation of the DecoratorManager is correct (well, if it could avoid notifying the repo view when a WorkingSet decoration has arrived, that'd be nice), the staging view must not assume it was the sole owner of that RepositoryNode. Therefore don't clear it; the view model RepositoryNode may still be used elsewhere.
New Gerrit change created: https://git.eclipse.org/r/151887
Gerrit change https://git.eclipse.org/r/151887 was merged to [master]. Commit: http://git.eclipse.org/c/egit/egit.git/commit/?id=09354fca29aefb20f03c9cde775905878b2b165f
P.S.: it's unclear to me why AbstractTreeViewer.associate() links foreign objects into its tree in the first place. There's a comment there that this was done because of bug "PR 1FV62BT". Where can I find out what that was? I didn't see similar code in AbstractTableViewer.
