[SOLVED] Clean up at TFileOpener.OpenMainUnit

[lagprogramming]

ide/sourcefilemanager.pas has

Code: Pascal  [Select][+][-]

1. function TFileOpener.OpenMainUnit: TModalResult;
2. var
3.   MainUnitInfo: TUnitInfo;
4. begin
5.   {$IFDEF IDE_VERBOSE}
6.   debugln(['[TFileOpener.OpenMainUnit] A ProjectLoading=',ofProjectLoading in Flags,' MainUnitID=',Project1.MainUnitID]);
7.   {$ENDIF}
8.   Result:=mrCancel;
9.   if (Project1=nil) or (Project1.MainUnitID<0) then exit;
10.   MainUnitInfo:=Project1.MainUnitInfo;
11.  
12.   // check if main unit is already open in source editor
13.   if (MainUnitInfo.OpenEditorInfoCount > 0) and (not (ofProjectLoading in FFlags)) then
14.   begin
15.     // already loaded -> switch to source editor
16.     SourceEditorManager.ActiveEditor := TSourceEditor(MainUnitInfo.OpenEditorInfo[0].EditorComponent);
17.     SourceEditorManager.ShowActiveWindowOnTop(True);
18.     Result:=mrOk;
19.     exit;
20.   end;
21.  
22.   // open file in source notebook
23.   Result:=OpenFileInSourceEditor(MainUnitInfo.GetClosedOrNewEditorInfo);
24.   if Result<>mrOk then exit;
25.  
26.   Result:=mrOk;
27.   {$IFDEF IDE_VERBOSE}
28.   debugln('[TFileOpener.OpenMainUnit] END');
29.   {$ENDIF}
30. end;

The following patch removes the lines:

Code: Pascal  [Select][+][-]

1.   if Result<>mrOk then exit;
2.  
3.   Result:=mrOk;

After if Result<>mrOk then exit; for sure Result is mrOk. If the IDE developers want custom verbose messages depending on the function result they can do it by adding the result value to the debugln parameter.
Here is the patch:

Code: Pascal  [Select][+][-]

1. diff --git a/ide/sourcefilemanager.pas b/ide/sourcefilemanager.pas
2. index a370bce746..5658d8a59d 100644
3. --- a/ide/sourcefilemanager.pas
4. +++ b/ide/sourcefilemanager.pas
5. @@ -1681,9 +1681,7 @@ begin
6.  
7.    // open file in source notebook
8.    Result:=OpenFileInSourceEditor(MainUnitInfo.GetClosedOrNewEditorInfo);
9. -  if Result<>mrOk then exit;
10.  
11. -  Result:=mrOk;
12.    {$IFDEF IDE_VERBOSE}
13.    debugln('[TFileOpener.OpenMainUnit] END');
14.    {$ENDIF}

[TRon]

If function returns mrRetry, old code is intented to return mrOk.

let's analyze that.

Code: Pascal  [Select][+][-]

1.   Result:=OpenFileInSourceEditor(MainUnitInfo.GetClosedOrNewEditorInfo);
2.   // as per your suggestion, result is mrRetry, mrCancel or whatever as long as it is not mrOK
3.  
4.   if Result<>mrOk then exit;  // we exit for all results (returning that result to the caller in the process), except when result is mrOK
5.  
6.   Result:=mrOk; // return mrOK to caller
7.  

So imho that can mean two situations:
1. OpenFileInSourceEditor is not (ever) able to return mrOK (or at a point in time it did not). Not likely but possible.
2. the code used to be different where the corrected/patched code made sense at that time.

[AlexTP]

After the patch, we have much more calls of

 debugln('[TFileOpener.OpenMainUnit] END');

[TRon]

After the patch, we have much more calls of

 debugln('[TFileOpener.OpenMainUnit] END');

True but was mentioned:

If the IDE developers want custom verbose messages depending on the function result they can do it by adding the result value to the debugln parameter.

So depending on what is preferred... ?

alternatively:

Code: Pascal  [Select][+][-]

1.   {$IFDEF IDE_VERBOSE}
2.   if Result = mrOk
3.     then debugln('[TFileOpener.OpenMainUnit] END');
4.   {$ENDIF}
5.  

[lagprogramming]

If function returns mrRetry, old code is intented to return mrOk.

let's analyze that.

Code: Pascal  [Select][+][-]

1.   Result:=OpenFileInSourceEditor(MainUnitInfo.GetClosedOrNewEditorInfo);
2.   // as per your suggestion, result is mrRetry, mrCancel or whatever as long as it is not mrOK
3.  
4.   if Result<>mrOk then exit;  // we exit for all results (returning that result to the caller in the process), except when result is mrOK
5.  
6.   Result:=mrOk; // return mrOK to caller
7.  

So imho that can mean two situations:
1. OpenFileInSourceEditor is not (ever) able to return mrOK (or at a point in time it did not). Not likely but possible.
2. the code used to be different where the corrected/patched code made sense at that time.

I don't get it.
For both versions of the function, after the line Result:=OpenFileInSourceEditor(MainUnitInfo.GetClosedOrNewEditorInfo); the result of OpenFileInSourceEditor is also the result of TFileOpener.OpenMainUnit.
The result of TFileOpener.OpenMainUnit remains unchanged in the proposed cleaned up version. In the official version, to the result of the TFileOpener.OpenMainUnit may be assigned a mrOk value only if the result is already mrOk, which makes the lines useless. Are you trying to say that the original code has a bug somewhere else?

After the patch, we have much more calls of

 debugln('[TFileOpener.OpenMainUnit] END');

Which is the expected information, according to the '[TFileOpener.OpenMainUnit] END' text.
Notice the presence of the two exits in the function. The original function doesn't always debugln that text. The original code doesn't always exit with a debugln. The text wasn't '[TFileOpener.OpenMainUnit] END with a mrOk result'. If the developers want to read that text line only when mrOk is returned, then they also have to insert a new line between Result:=mrOk; and exit; at if (MainUnitInfo.OpenEditorInfoCount > 0) and (not (ofProjectLoading in FFlags)). If they want to read the line when Result:=mrCancel;if (Project1=nil) or (Project1.MainUnitID<0) then they have to insert new code there, too.
I think Lazarus developers should be announced that the debugln text regarding the exit of the function isn't always shown. Maybe they want to completely remove it, or maybe they want to add new ones at previous exits. They might decide to leave it as it is.

[TRon]

Are you trying to say that the original code has a bug somewhere else?

If addressed to me then the answer is no. I was disputing the remark from AlexTP that your changes will change (functional) behaviour of the function (which imho it will not, especially not after my proposed alternative).

BTW: is there a particular reason that you post your proposals/patches here instead of using the bugtracker ? Not that it matters much but it just comes across as a little strange, that's all as you are no newbie (anymore).

[JuhaManninen]

I removed the obsolete code. Thanks for noticing.