Bug 489856 - [Model] Track whether parts are on top in the UI
Summary: [Model] Track whether parts are on top in the UI
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.6   Edit
Hardware: PC All
: P3 normal with 2 votes (vote)
Target Milestone: ---   Edit
Assignee: Platform-UI-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 489808
  Show dependency tree
 
Reported: 2016-03-17 10:40 EDT by Simon Scholz CLA
Modified: 2016-09-23 09:22 EDT (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Scholz CLA 2016-03-17 10:40:55 EDT
In Bug 489808 I'd also like to address pausing and resuming ISideEffect within a part. Therefore I need to track, which part is on top, this means the user can see its contents in the UI.

In order to achieve this I'd like to introduce a onTop tag, which is applied to parts and placeholder of parts.

So I could react on these tag changes in my SideEffectAddon of Bug 489808 in order to pause or resume ISideEffects within a part.
Comment 1 Eclipse Genie CLA 2016-03-17 17:44:27 EDT
New Gerrit change created: https://git.eclipse.org/r/68725
Comment 2 Thomas Schindl CLA 2016-03-17 18:03:03 EDT
I'm not sure tags are a good idea (eg your current patch is broken with save & restore if not mistaken because you now might have multiple onTop-Tags). 

A performance problem with tags is that they immediately lead to CSS-Style-Reapply which might or might not be desired!

What about recording the information in the IEclipseContext associated with the MPart so one could even react on that information via injection in the part itself (infact this would make the ISideEffect-Addon useless as the CF could create an MPartSideEffects and use @Inject setOnTop(@Named("onTop") boolean onTop) there)

Even further I think no addon is needed at all because if the 'onTop' concept is made a core e4 concept it would be implemented in the IPresentation-Engine - hint you anyways added the constant there!
Comment 3 Simon Scholz CLA 2016-03-18 07:34:03 EDT
(In reply to Thomas Schindl from comment #2)
> I'm not sure tags are a good idea (eg your current patch is broken with save
> & restore if not mistaken because you now might have multiple onTop-Tags). 

You're right, when the application is launched I could cleanup the onTop tags before applying them.
In general having multiple onTop tags should be the case, since more than one part can be on top.

> A performance problem with tags is that they immediately lead to
> CSS-Style-Reapply which might or might not be desired!

The MixMaxAddon does it in a similar way. And styling onTop elements might be desired. I know this is already done somehow with the swt-selected-tab-fill css property.
Anyways these performance problem are really annoying, but this concludes to suggest avoiding tags, which would be bad since tags in general are a nice idea.
 
> What about recording the information in the IEclipseContext associated with
> the MPart so one could even react on that information via injection in the
> part itself (infact this would make the ISideEffect-Addon useless as the CF
> could create an MPartSideEffects and use @Inject setOnTop(@Named("onTop")
> boolean onTop) there)
> 
> Even further I think no addon is needed at all because if the 'onTop'
> concept is made a core e4 concept it would be implemented in the
> IPresentation-Engine - hint you anyways added the constant there!

Having an addon would decouple the onTop behaviour from a particular IPresentationEngine, so you could consume it automatically in e(fx)clipse without changing your IPresentationEngine implementation.

I also like the idea to put this onTop value into the context of MContext elements and creating ICompositeSideEffects with the ContextInjectionFactory, so that they can use @Inject setOnTop(@Named("onTop") and @PreDestroy.

With this approach we could have something like this:

public class DICompositeSideEffect implements ICompositeSideEffect {

	private ICompositeSideEffect compositeSideEffect;

	public DICompositeSideEffect() {
		compositeSideEffect = ICompositeSideEffect.create();
	}

	public DICompositeSideEffect(ICompositeSideEffect wrappedCompositeSideEffect) {
		assert (wrappedCompositeSideEffect != null);
		compositeSideEffect = wrappedCompositeSideEffect;
	}

	@Inject
	@Optional
	public void setOnTop(@Named(IPresentationEngine.ON_TOP) Boolean onTop) {
		if (onTop.booleanValue()) {
			resumeAndRunIfDirty();
		} else {
			pause();
		}
	}

// ... all other methods of ICompositeSideEffect
}

What do you think about keeping the addon and let it manage the onTop Boolean of MPart or MContext elements? This encapsulates this behaviour in the addon or could you point at a proper place in the IPresentationEngine?
By using an DICompositeSideEffect, which can react on the changes, the ISideEffect-Addon could then be omitted.
Comment 4 Thomas Schindl CLA 2016-03-18 07:51:01 EDT
I think that the concept of having a part "onTop" (not sure the term is correct) should be core concept one can rely on ALWAYS when writing e4 applications (the IDE is just a big one) to improve performance without doing a lot of calculations yourself.

I should not only get this feature if the respective addon is there and it is different to other addons (eg MinMax, DnD) who add a new visual feature eg an IDE wants to ship but many RCPs don't want/need it because it confuses their users hence.

If you take this into account I think its wrong to implement that in an addon but needs to go into the core layer, I have to revise my advice of putting into into IPresentationEngine-Impl as you could naturally implement this stuff as well in E4Workbench which any e4 implementation uses anyways.
Comment 5 Stefan Xenos CLA 2016-03-30 15:02:27 EDT
I think E3 parts can do this via IPartListener. Maybe there's some way to expose it to E4 as well?
Comment 6 Simon Scholz CLA 2016-04-08 07:00:45 EDT
@Tom can you have a look at the current patch and give feedback whether this suits your suggestions?

@Stefan This patch will work for both E3 and E4.
Comment 8 Noopur Gupta CLA 2016-09-23 08:43:08 EDT
(In reply to Eclipse Genie from comment #7)
> Gerrit change https://git.eclipse.org/r/68725 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/
> ?id=2539fb0e3f94ec21e81df44519cccc1e8ee7e916

IWorkbench.ON_TOP - the @since tag should be 1.5.
Comment 9 Eclipse Genie CLA 2016-09-23 09:04:36 EDT
New Gerrit change created: https://git.eclipse.org/r/81790
Comment 10 Simon Scholz CLA 2016-09-23 09:07:29 EDT
(In reply to Noopur Gupta from comment #8)
> (In reply to Eclipse Genie from comment #7)
> > Gerrit change https://git.eclipse.org/r/68725 was merged to [master].
> > Commit:
> > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/
> > ?id=2539fb0e3f94ec21e81df44519cccc1e8ee7e916
> 
> IWorkbench.ON_TOP - the @since tag should be 1.5.

Thank you Noopur, it's been a while since I created that patch and today I only rebased it.

I am wondering wheater our Hudson was able to check these @since tags during the build as well, because in my IDE with the correct baseline I can see this issue immediately. But not when I just rebase in the gerrit web UI.