If function returns mrRetry, old code is intented to return mrOk.
let's analyze that.
Result:=OpenFileInSourceEditor(MainUnitInfo.GetClosedOrNewEditorInfo);
// as per your suggestion, result is mrRetry, mrCancel or whatever as long as it is not mrOK
if Result<>mrOk then exit; // we exit for all results (returning that result to the caller in the process), except when result is mrOK
Result:=mrOk; // return mrOK to caller
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.