Skip to content
This repository has been archived by the owner on Oct 4, 2021. It is now read-only.

Only brings ErrorListPad to front in case of be loaded in Workbench #9121

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

netonjm
Copy link
Contributor

@netonjm netonjm commented Oct 28, 2019

Only brings ErrorListPad to front in case of be loaded in Workbench

Fixes #941969 - [FATAL] System.NullReferenceException exception in MonoDevelop.MacIntegration.MainToolbar.BuildResultsView.MouseDown()

@netonjm netonjm requested review from mrward and sevoku October 28, 2019 17:17
Copy link
Member

@mrward mrward left a comment

Choose a reason for hiding this comment

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

Do we know why the ErrorPad is null here?

It reminds me of that other null reference exception where the ErrorPad is null when this should not be possible since it is part of MonoDevelop.Ide.

@Therzok
Copy link
Contributor

Therzok commented Oct 29, 2019

Looking at WrapPad:
http://source.monodevelop.com/#MonoDevelop.Ide/MonoDevelop.Ide.Gui/Workbench.cs,28ef42ba3aee7408

It seems that the pad is removed from the list when destroyed.

GetPad<T> goes through Pads which is only constructed once. So it is possible for it to be null because someone destroyed the window (via the x button)

@Therzok
Copy link
Contributor

Therzok commented Oct 29, 2019

I'm pretty sure we should be reconstructing the pad if not found.

@netonjm
Copy link
Contributor Author

netonjm commented Oct 29, 2019

Yeah @Therzok, I think it depends on what behaviour we want in case of the user closes the panel (and it's destroyed).

In theory (please correct me if I'm wrong), this brings to the front the pad when build finishes the panel to show the build summary (errors/etc..) then here the question is do want always want to show it even though the user closes it?

For me makes sense as @Therzok says to force open since it is quite valuable information. thoughts @sevoku ?

I'm curious if being destroyed when opening it again will have the information of the finished build

@netonjm netonjm self-assigned this Oct 29, 2019
@mrward
Copy link
Member

mrward commented Oct 29, 2019

In this case the user has clicked the error or warning icon in the status bar so we should always open the Errors window. If that means we need to re-create the Errors window then we should do that and then display it. Doing nothing would not be a good user experience.

There is a question of where this logic should go. I think the IdeApp.Workbench.GetPad should recreate the pad, if it needs to, since it knows about them instead of having the logic in several places.

@netonjm
Copy link
Contributor Author

netonjm commented Oct 29, 2019

Makes completely sense @mrward @Therzok ! click the icon is an action entirely linked on this pad and it should recreate the panel absolutely! I was on a wrong premise.

And related to the IdeApp.Workbench.GetPad recreate, not completely sure if include it here, maybe I would include another method IdeApp.Workbench.GetOrCreatePad reusing the other logic

@netonjm
Copy link
Contributor Author

netonjm commented Dec 2, 2019

I was investigating with @mrward , and we don't have any kind of dispose or remove the panel.
Hide panel don't destroy the object, it seems we only dispose panels on close the IDE. I added some additional logging in case of NRE to get more information related to that

@netonjm netonjm requested a review from mrward December 9, 2019 11:32
Copy link
Member

@mrward mrward left a comment

Choose a reason for hiding this comment

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

Just wondering if we could capture some extra information here to help diagnose the problem. This change will prevent a fatal exception causing a crash and still reports the problem in telemetry 👍

@netonjm
Copy link
Contributor Author

netonjm commented Jan 7, 2020

@monojenkins rebase

Base automatically changed from master to main March 9, 2021 14:17
@akoeplinger akoeplinger changed the base branch from main to master March 15, 2021 17:01
Base automatically changed from master to main March 15, 2021 17:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants