[Bug] Pictures Showing EXIF Details hangs MP [Mantis #4465] (1 Viewer)

[elliottmc]

I agree that we need to look into this a bit more. On the other hand, I haven't seen any other reports related to dialogs, and I haven't seen any issues in my own testing. Scythe doesn't seem to be around much at the moment, so perhaps we should consider committing this fix for 1.4.0, and then looking into the deeper issues for 1.5.0 ?

Being pragmatic yes but this could also affect plugins so there could be dialogs in any number of plugins that are broken. I guess as long as we test where we can (and I guess more people would have reported issues they were seeing).

There are so many places where dialogs are called that is nearly impossible to test them all...

I completely agree. However, I would say that of the many dialogs in MP, this one probably gets used less than most, and yet this is the only one that we have a reported error for.

I think we have to fix what we can, test what we can and then to a certain extent hope for the best.

 

[jameson_uk]

I would say that of the many dialogs in MP, this one probably gets used less than most, and yet this is the only one that we have a reported error for.

I think we have to fix what we can, test what we can and then to a certain extent hope for the best.

The issue is not the dialog per se but the way it is coded. In this example I think you could set the dialog title and it would work but if you don't set a title it would not. Now this dialog could be called from four different locations and if two of them set a title and two do not then it would work half the time.

Now I have simply moved some resource allocation code from the set title method to the DoModal method but I have no idea whether this could cause and issue if someone did call the set title method (it is not called for EXIF dialog anywhere in MP code).

The bigger issue is the yes / no dialog. IIRC this is the same and is obviously called all over the shop. Now this might set the title 99 times out of a hundred and leaving the one other time as an error (which by the looks of it worked in 1.3). Now extend this to plugins and you can see the issue.

Thinking about it we should probably see what happens if we call set title for the EXIF dialog. If this does not throw any exceptions or cause any issues we should look at moving the resource allocation for all dialogs from the set title code to the DoModal method (this is what is called to actually display the dialog so must be called every time). Unfortunately I won't have time for this for the next few weeks but if someone could look at this would be a much better solution

 

[elliottmc]

Do you think this needs to be done for 1.4.0 ?

 

[jameson_uk]

Do you think this needs to be done for 1.4.0 ?

I guess the key thing here is that no-one has found any other dialogs that are not currently working.
The set title test should be a relatively simple thing to do. If this works then we can safely fix all the dialogs and be confident they will work in all situations.
Just needs someone with a little time. If there is no-one then for 1.4 we might as well fix the known issue as we know the fix is good

 

[Sebastiii]

Can you give an example ? about the good way to do ?
Thanks

 

[jameson_uk]

Can you give an example ? about the good way to do ?
Thanks

The current code in GUIDialogExif for SetHeading is

	public void SetHeading(string strLine)
	{
	  //LoadSkin();
	  AllocResources();
	  InitControls();
 
	  lblHeading.Label = strLine;
	}

but I have removed all the code for the heading and moved the calls for AllocResources and InitControls to DoModal which is fine for the one place we create this dialog internally but for other dialogs we need the heading.

My hunch is that if we had

	public void SetHeading(string strLine)
	{
	  lblHeading.Label = strLine;
	}

in GUIDialogExif and call this prior to DoModal then we will end up with a null reference exception because lblHeading would not be created yet.

If you look at DialogOK this is the same; AllocResources and InitControls are called in SetHeading and should be easier to spot an issue there.

So what tests do we need to do... I would suggest that for dialogs

TEST 1

• Find some code where we create a simple dialog (GUIOK / GUIYesNo etc).
• Check we are currently calling SetHeading prior to DoModal
• Comment out the call to SetHeading
• See if when this runs we get a null reference exception or does MP not actually display anything?
• Try this against 1.3 codebase. Has this always been the case or is this some regression?

TEST 2

• Find same code as above (from master not modified code)
• Update GUIDialogOK to move AllocResources and InitControls from SetHeading to DoModal
• See if when run we get a null reference exception when SetHeading is called ?
• Try this against 1.3 codebase.

What I don't know is whether this problem relates to delayed loading of skin files (from GUIWindow default is delayed)

	public virtual bool SupportsDelayedLoad
	{
	  get { return true; }
	}

My guess is this must only be dialogs else we would have noticed this a lot more but I don't understand why.... This is right down at the resource allocation level and I get a bit lost as the code is very confusing right down there....

 

[mbuzina]

Will retest using the installer from first post - downloading it now.

 

[mbuzina]

OK - I fully removed MP and installed the version from the first post and now the issue seems to be fixed! Maybe the installation I tried on before was wrong (initially I replaced the dialog.dll and later tried a re-install with installer from first post).

But the good news is: It works (have not tested for regressions yet).

 

[Sebastiii]

Thanks Mike, i will try to do that

@mbuzina, i have push a fix about random slideshow, when doing ESC key it should select the correct item and it should also fix repeat image and fix blank screen.

I didn't build any installer but it could be nice if you can try.

Arg that not the correct thread for it.

I will PM you link later

 

[mbuzina]

OK, no prob - will test