Page MenuHomePhabricator

efl_ui_win: add 'exit_on_all_windows_closed' class property and unit test
ClosedPublic

Authored by zmike on Jan 10 2019, 12:59 PM.

Details

Summary

this property causes the main loop to exit with the passed exit code
when the standby event is triggered

@feature
ref T5494

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
zmike created this revision.Jan 10 2019, 12:59 PM
cedric accepted this revision.Jan 10 2019, 1:31 PM
This revision is now accepted and ready to land.Jan 10 2019, 1:31 PM

I somehow don't like to have a function with windows in its name in efl_app.eo efl_app is in the core of EFL, the core of EFL does not neccessarily have anything to do with windows.

How about this function in a config class in the Efl.Ui namespace, which would generate a STANDBY event, on the loop you can decide if you want to exit on STANDBY or not, would sound a bit nicer and cleaner to me, since the two different concernes (Application-Runtime and Action-on-windows-closed) are seperated. Additionally, this design could allow a none UI efl app to use the STANDBY event for the case that no client or so is connected ... :)

I somehow don't like to have a function with windows in its name in efl_app.eo efl_app is in the core of EFL, the core of EFL does not neccessarily have anything to do with windows.

How about this function in a config class in the Efl.Ui namespace, which would generate a STANDBY event, on the loop you can decide if you want to exit on STANDBY or not, would sound a bit nicer and cleaner to me, since the two different concernes (Application-Runtime and Action-on-windows-closed) are seperated. Additionally, this design could allow a none UI efl app to use the STANDBY event for the case that no client or so is connected ... :)

I don't like the side config class concept very much. It is really a side thing that don't seem to match the rest of the infrastructure. I am not sure I also read you correctly, but I think you agree on STANDBY event on the Efl.App object. So the only question is where is this property get/set defined? In that case what do you think if it was a class property on Efl.Ui.Win ?

I don't like the side config class concept very much. It is really a side thing that don't seem to match the rest of the infrastructure. I am not sure I also read you correctly, but I think you agree on STANDBY event on the Efl.App object. So the only question is where is this property get/set defined? In that case what do you think if it was a class property on Efl.Ui.Win ?

You read me correctly. Having it as class function on efl.ui.win sounds actaully not *that* wrong to me (it cannot be on the object, as all objects are defining 1 property, not every obejct 1 property). The reason why i liked the idea of a Efl.Ui.Temp_Config/App_Config is, that we likely will have more config values there, which are right now in elm_config but don't make really sense there, Just a few from my head:

  • A software came definitly wants to have hardware accel, or something that does heavy UI stuff, so you probebly want to have your app beeing accelated, without setting the whole system to hardware accelation
  • elm_config_selection_unfocused_clear_get (also applies to other focus properties) really depends on the UI you have and you probebly don't want to have that behaviour

So i would say that Efl.Ui.Temp_Config / App_Config is just a class to put all those properties instead of putting them in the logical units in the tree. Which goes pretty much hand in hand with Efl.Ui.Config.

So all in all our config concept in EFL would look like:
Efl.Ui.Config <- configs that are loaded / stored into a file on the fs
Efl.Ui.App_Config <- configs that are setted by the app, propebly defaulted by normal Efl.Ui.Config properties, but the setted values are *never* applied to any fs. In general, a place to alter the behaviour of the UI parts of the efl framework.

I don't like the side config class concept very much. It is really a side thing that don't seem to match the rest of the infrastructure. I am not sure I also read you correctly, but I think you agree on STANDBY event on the Efl.App object. So the only question is where is this property get/set defined? In that case what do you think if it was a class property on Efl.Ui.Win ?

You read me correctly. Having it as class function on efl.ui.win sounds actaully not *that* wrong to me (it cannot be on the object, as all objects are defining 1 property, not every obejct 1 property). The reason why i liked the idea of a Efl.Ui.Temp_Config/App_Config is, that we likely will have more config values there, which are right now in elm_config but don't make really sense there, Just a few from my head:

  • A software came definitly wants to have hardware accel, or something that does heavy UI stuff, so you probebly want to have your app beeing accelated, without setting the whole system to hardware accelation
  • elm_config_selection_unfocused_clear_get (also applies to other focus properties) really depends on the UI you have and you probebly don't want to have that behaviour

I am not a fan of the Config interface as it looks really like dropping stuff together as an easy solution. Current elm_policy kind of show that issue. For example, having a window being accelerated, should be the result of a constructor property I think, more than a config things (As it doesn't affect the general default in that situation). I don't know what the other function does, maybe it make sense to have a config for that one, but will have to be convinced. Still in the current scenario, I think a @class function is what we can agree on and move forward with.

So i would say that Efl.Ui.Temp_Config / App_Config is just a class to put all those properties instead of putting them in the logical units in the tree. Which goes pretty much hand in hand with Efl.Ui.Config.

So all in all our config concept in EFL would look like:
Efl.Ui.Config <- configs that are loaded / stored into a file on the fs
Efl.Ui.App_Config <- configs that are setted by the app, propebly defaulted by normal Efl.Ui.Config properties, but the setted values are *never* applied to any fs. In general, a place to alter the behaviour of the UI parts of the efl framework.

I think that could be part of a general overall of our Config interface in Efl.Ui. At this point, I think this is a lot of work to get it right and not critical with the API we are trying to stabilize.

zmike edited the summary of this revision. (Show Details)
zmike requested review of this revision.Tue, Jan 29, 12:06 PM
zmike updated this revision to Diff 19023.

I had to rebase this so it needs review again

bu5hm4n requested changes to this revision.Wed, Jan 30, 2:54 AM

Well - the review above came to the conclusion that htis should rather be a class function on efl.ui.win rather than efl_app, (because efl_app does not have a lot to do with a UI)

This revision now requires changes to proceed.Wed, Jan 30, 2:54 AM
zmike added a comment.Wed, Jan 30, 5:57 AM

So then probably this patch should be abandoned since there's a separate patch to do that?

zmike updated this revision to Diff 19076.Wed, Jan 30, 12:30 PM
zmike retitled this revision from efl_app: add 'exit_on_windows_close' property and unit test to efl_ui_win: add 'exit_on_windows_close' class property and unit test.
bu5hm4n accepted this revision.Wed, Jan 30, 1:19 PM
bu5hm4n added a subscriber: segfaultxavi.

What the heck happened this time, I am seeing everywhere "Context not available" ... o_O

Looks good to me, @segfaultxavi is there something about docs ? @cedric is this fine to you ? :)

src/lib/elementary/efl_ui_win.eo
661

I am not a native speaker, but is that a real sentence ?

This revision is now accepted and ready to land.Wed, Jan 30, 1:19 PM
segfaultxavi requested changes to this revision.Thu, Jan 31, 2:17 AM

How is this property supposed to work?
You set this property to the Eina_Value that will be returned on exit, but how do you enable or disable the app quitting on all windows closed?
I know I missed the whole discussion, but this comes to show you the docs are insufficient :)

Also, I have the same comment as @bu5hm4n regarding the first line in the property doc.

This revision now requires changes to proceed.Thu, Jan 31, 2:17 AM
zmike updated this revision to Diff 19111.Thu, Jan 31, 5:32 AM
zmike edited the summary of this revision. (Show Details)

doc update

zmike added a comment.Thu, Jan 31, 5:32 AM

It is a real sentence.

The docs for exit_on_close (D7595) and exit_on_windows_close (D7594) are awfully similar. I still don't understand the difference, so I think it is mandatory that these two properties are compared to each other in their docs. Something like:

Compare with @.exit_on_close where the app only exits on Mondays, whereas @.exit_on_windows_close will exit any day of the week.
zmike added a comment.Thu, Jan 31, 7:08 AM

The difference is that one is a property on a specific window and one is global.

Understood. If you put that same sentence in the docs for both properties nobody will bother you again requesting clarification.

zmike added a comment.Thu, Jan 31, 7:38 AM

Understood. If you put that same sentence in the docs for both properties nobody will bother you again requesting clarification.

Which sentence?

segfaultxavi accepted this revision.Thu, Jan 31, 7:55 AM

I'll address any further concern I might have regarding docs on separate patches.

This revision is now accepted and ready to land.Thu, Jan 31, 7:55 AM
zmike updated this revision to Diff 19114.Thu, Jan 31, 8:20 AM
zmike retitled this revision from efl_ui_win: add 'exit_on_windows_close' class property and unit test to efl_ui_win: add 'exit_on_all_windows_closed' class property and unit test.

rename

This revision was automatically updated to reflect the committed changes.