Improve 'EditorStack' detection in CTabRendering

[praveen-skp]

Issue: #1570.

The current CTabRendering check if a tab is part of editor area, by checking if the tab has the 'EditorStack' tag. If the tag is missing, then the tab is considered as part of view area.
This fix enhances the tab identification by including a check based upon the location, even when the 'EditorStack' tag is absent.

This is a proposal, since this implementation is not working because the injection does not work and so, the getElementLocation throws null pointer exception.

[praveen-skp]

Another approach is by adding a getModelService method in the StackRenderer and use the following code to get the locaiton.
int location = ((StackRenderer) element.getRenderer()).getModelService().getElementLocation(element);

[iloveeclipse]

This is a proposal, since this implementation is not working

I've converted this PR to draft.

[laeubi]

@praveen-skp model elements should implement MContext#getContext() where you can get an IEclipseContext you can use to lookup the required service.

[laeubi]

I don't think this is correct, there can be many Area(s) in a RCP application and kust because they are shared don't make them the editor area.

[BeckerWdf]

But have a look into StackRenderer#createWidget. There we have the same check:

		// Adjust the minimum chars based on the location
		int location = modelService.getElementLocation(element);
		if ((location & EModelService.IN_SHARED_AREA) != 0) {
			tabFolder.setMinimumCharacters(MIN_EDITOR_CHARS);
			tabFolder.setUnselectedCloseVisible(true);
		} else {
			tabFolder.setMinimumCharacters(MIN_VIEW_CHARS);
			tabFolder.setUnselectedCloseVisible(false);
		}

[laeubi]

It is obviously not the same check ;-)

But if we are sure we want to use the same the EditorTag check can completely be removed as it would be redundant then. Please note that usually there is only one editor Area but different part stacks.

So if this one is MISSING the Editor is just added to a ramdom Partstack, its good that this is happily the "right" area here... but this does not fix the missing EditorTag.

[BeckerWdf]

It is obviously not the same check ;-)

Really?

(location & EModelService.IN_SHARED_AREA) != 0) and
(location & EModelService.IN_SHARED_AREA) != 0) look quite identical for me. Am I overlooking something?

[laeubi]

(element != null && element.getTags().contains(EditorTag))
is not the same as
(location & EModelService.IN_SHARED_AREA) != 0)

so who ever did the tag check instead of the location check has did this probably for a purpose, now the EditorTag check even seem to become obsolete as the EditorArea is almost always shared (I can't think about a reason why one don't want this), also as I mentioned Eclipse uses the ID to determine it is "the editor stack" so why not consider this as well and so on.

[merks]

The following works for me:

	private boolean isPartOfEditorStack() {
		MUIElement element = (MUIElement) parent.getData(AbstractPartRenderer.OWNING_ME);
		EObject root = EcoreUtil.getRootContainer((EObject) element, true);
		if (root instanceof MContext context) {
			EModelService eModelService = context.getContext().get(EModelService.class);
			if (eModelService != null) {
				int location = eModelService.getElementLocation(element);
				return (location & EModelService.IN_SHARED_AREA) != 0;
			}
		}
		return false;
	}

EcoreUtil.getRootContainer is null safe.

It looks like this where otherwise all the tabs in both areas would be squished:

[BeckerWdf]

The following works for me

So does this mean we have consensus that the PR with the changes proposed by @merks are good and would fix the issue (even when it does not fix the root cause of the disappearing tag)?

[merks]

Given that this can be demonstrated to fix the current problem, I think the worst thing that could happen is that in some places the tabs might not be squished where you guys want them squished. But my opinion is just one opinion...

[HeikoKlare]

I can confirm that the fix proposed by Ed solves the problem for me. And I agree that avoiding squished editor tabs outweighs the risk of some view tabs not being squished when they should be.

[merks]

You are trying to use the injected field during construction but it will be injected only after construction:

[laeubi]

You are trying to use the injected field during construction but it will be injected only after construction:

E4 also supports constructor injection (just in case), but I think the fix is invalid anyways.

[github-actions]

Test Results

   918 files  ±0     918 suites  ±0   1h 8m 0s ⏱️ + 19m 29s
 7 434 tests ±0   7 284 ✅ +3  150 💤 ±0  0 ❌  - 3 
23 451 runs  ±0  22 949 ✅ +3  502 💤 ±0  0 ❌  - 3 

Results for commit 1a18c6d. ± Comparison against base commit e630919.

♻️ This comment has been updated with latest results.

[merks]

I'm not sure the following observations are best made in the issue or here in this PR.

When trying to reproduce the cause of the problem I already notice this problem, i.e., if you drag an editor to create a split editor area, the second area's tabs are squished:

If you now close all the editors and open new editors, those tabs are squished as well:

So it's highly likely that anyone who ever create such a split editor area will always see this problem and will them permanently see this problem...

[BeckerWdf]

I just updated the PR with @merks proposal.
Feel free to approve and merge once CI is green.

[HeikoKlare]

I must not merge during freeze period, but I guess @merks will do?

[merks]

@iloveeclipse

I just wanted to double check that you too are fine with merging this. It has my approval on behalf of the PMC...

[iloveeclipse]

I just wanted to double check that you too are fine with merging this. It has my approval on behalf of the PMC...

I'm not familiar with this code. Any chance to have a test case added?

[merks]

If someone else wants to a test case, which I think is relatively challenging, they may do so, but I personally don't have the time for that. I believe right now such time would likely be better invested in figuring out why the editor stack area tag is getting lost and while doing that, create a test case for that more general problem which has gone undetected until now...

In any case, I don't want that to hold up getting this into an I-Build.

[BeckerWdf]

I believe right now such time would likely be better invested in figuring out why the editor stack area tag is getting lost and while doing that, create a test case for that more general problem which has gone undetected until now...

I second that. Once we have fixed the root cause we can then also remove this workaround here again.
Thanks to @merks for approving and merging.

[merks]

I still wonder about the case where the second editor area's tab are squished. If makes me wonder if any such new editor area should have a tag. But indeed, that is a separate issue that is significantly harder to address, though I see @HeikoKlare ❤️ has made some interesting progress as a starting point!