Foreground detection mixin service for widgets

[digitaldan]

Signed-off-by: Dan Cunningham dan@digitaldan.com

[relativeci]

Job #444: Bundle Size — 10.75MB (~-0.01%).

96f4513 vs ef38431

Changed metrics (2/10)

Metric	Current	Baseline
Initial JS	1.67MB(-0.05%)	1.68MB
Cache Invalidation	18.56%	22.64%

Changed assets by type (1/7)

	Current	Baseline
JS	8.69MB (~-0.01%)	8.69MB

---

View Job #444 report on app.relative-ci.com

[digitaldan]

Hi @ghys, this is your solution as discussed in #1390 . Since this is needed for more then just the video widget (image widget as well), i was thinking making it an optional mixin for those types of widgets might be a good option? I'm importing this now in the WebRTC widget which works nicely.

[hubsif]

Good idea to provide this in a mixin 👍. If it works as expected from what I read in #1390, I'm going to use it for my sip widget as well.

[digitaldan]

Thanks @hubsif , to implement you do something like this in your widget

import foregroundService from '../widget-foreground-service'

export default {
  mixins: [foregroundService],
  name: 'OhVideoWebRTC',
  methods:  {
      startForegroundActivity () {
        this.startPlay()
      },
      stopForegroundActivity () {
        this.stopPlay()
      }
  }
}

there is also a variable isForeground which is true if it's in the foreground, i use this when a property changes (my src value) and i need to know if i should start the video or not.

[ghys]

Yes, thank you! I had an idea of adding this to the widget mixin at first but subscribing to events unnecessarily was not the right way to go; making it separate is a good option.

A minor remark is that startForegroundActivity or stopForegroundActivity could be called needlessly, it's not a problem as long as their implementations take this into account - so for instance startForegroundActivity should detect if the activity is already started and do nothing in that case.

[ghys]

Could you elaborate on this? I tested the original code with popups and found out that it seemed to work as expected?

[digitaldan]

Yeah, it was working for me as well when i had the logic directly in my widget , but when using as a mixin, it was not finding the page-current class.

I actually can't test this right now to show some examples, the build seems to be broken? I rebased to main and now I am getting the error when running npm start:

Module not found: Error: Can't resolve 'source-map-loader' in '/Users/daniel/openhab-main/git/openhab-webui/bundles/org.openhab.ui/web'

Along with a whole lot of other source-map-loader errors. I tried blowing away my node modules and reinstalling, and when that did not work, i cloned a clean copy of the repo, did a npm install then npm start and am seeing the same error.

[digitaldan]

So it looks like this line

openhab-webui/bundles/org.openhab.ui/web/build/webpack.config.js

Line 92 in f1851e9

use: (env === 'development' || buildSourceMaps) ? ["source-map-loader"] : [],

which was introduced in commit f1851e9 is causing my local build to fail, simply removing this test works FYI.

[digitaldan]

so back to the topic at hand, if i dump out the this.$el.closest('.page') to the console during a popup, you can see it does not contain the page-current class. I'm not sure why thats different now, maybe a loading order issue?

[ghys]

Along with a whole lot of other source-map-loader errors. I tried blowing away my node modules and reinstalling, and when that did not work, i cloned a clean copy of the repo, did a npm install then npm start and am seeing the same error.

Merged #1401 too fast I suppose - it only affects development and npm i source-map-loader should also solve the problem for the time being (but the package.json should obviously include it so it's installed).

so back to the topic at hand, if i dump out the this.$el.closest('.page') to the console during a popup, you can see it does not contain the page-current class.

That would be another overlook - I'm assuming you want the widget to work when it's in a page presented as a popup. The original code works correctly when it's on a regular page and you open a popup over it; the activity (like playing a video) would be expected to continue because it might still be visible behind the popup, especially on desktops. And it does, because the pageBeforeOut event is not called on the top-level page since it doesn't go away, another page is simply displayed on top (in the popup). Also the page-current class is apparently only for top-level pages, and is not applied to popups.

So this "page-current class means the page is already is the foreground" logic is wrong because it doesn't work with modals. Note that popups are not the only type of modals (sheets and popovers, albeit far less used) can be encountered as well.

[digitaldan]

and npm i source-map-loader should also solve the problem for the time being

Yeah, that was the first thing i tried, but it led to some package conflict which i thought could happen when your npm cache is out of sync (it was not the issue in this case), so when trying from a fresh clone failed, I kinda gave up debugging the issue :-)

That would be another overlook - I'm assuming you want the widget to work when it's in a page presented as a popup.

i have only tried this PR running the widget in popups and in full pages, but not a page in a popup (or other modals)

So this "page-current class means the page is already is the foreground" logic is wrong

So does that mean this technique is not viable, or needs to be tweaked in your opinion?

[ghys]

Continuing the conversation above with a suggestion:

[ghys]

Suggested change

	mounted () {
	// determine the current page - support: https://caniuse.com/?search=closest
	this.pageEl = this.$el.closest('.page')
	const isPopup = this.$el.closest('.popup') !== null
	// start the foreground activity immediately if the page
	// is already in the foreground when the widget is mounted
	// or is being shown within a popup
	if (this.pageEl.classList.contains('page-current') || isPopup) {
	this.inForeground = true
	this.startForegroundActivity()
	}
	this.$f7.on('pageAfterIn', this.onPageAfterIn)
	this.$f7.on('pageBeforeOut', this.onPageBeforeOut)
	mounted () {
	const isInModal = this.$el.closest('.framework7-modals')
	if (isInModal) {
	this.startForegroundActivity()
	return
	}
	this.pageEl = this.$el.closest('.page')
	if (this.pageEl && this.pageEl.classList.contains('page-current')) {
	this.startForegroundActivity()
	}
	this.$f7.on('pageAfterIn', this.onPageAfterIn)
	this.$f7.on('pageBeforeOut', this.onPageBeforeOut)

[ghys]

Yeah, that was the first thing i tried, but it led to some package conflict which i thought could happen when your npm cache is out of sync (it was not the issue in this case), so when trying from a fresh clone failed, I kinda gave up debugging the issue :-)

This should probably be addressed asap as it's only useful to @stefan-hoehn at the moment and breaks the build for others.

So does that mean this technique is not viable, or needs to be tweaked in your opinion?

It's just a matter of making it work in as many combinations of pages & modals as possible. It would appear at mounting time modals are in the DOM under a background div with the framework7-modals class (then they're moved elsewhere). There's no need to subscribe to events when in a modal, since they are temporary in nature and destroyed when closed. A caveat is that a modal may be covered by another modal (or navigate to a page while still in the background) but in this case the foreground activity won't stop.
The suggestion above seems to work in most cases:

[ghys]

@digitaldan as the feature freeze is imminent would be able to address the minor change above and use the mixin in #1386 in time?

[ghys]

@digitaldan as you're busy with the iOS app as discussed I finished this PR to include my earlier remark about modals. This PR can be safely merged because it is not currently used and won't even be compiled in before #1386 uses it.