Page MenuHomePhabricator

efl.ui.popup
Closed, ResolvedPublic

Description

class Efl.Ui.Popup @beta
├ (P) align
├ (P) timeout
├ (E) backwall,clicked
├ (E) timeout

Action Items

  • Migrate align and popup_size to Efl.Gfx.Size_Hints min/max_size and align

Related Objects

StatusAssignedTask
Resolvedzmike
Resolvedzmike
Resolvedzmike
Resolvedzmike
ResolvedNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
ResolvedNone
OpenNone
ResolvedNone
ResolvedNone
bu5hm4n created this task.May 3 2019, 12:06 PM
bu5hm4n triaged this task as TODO priority.

I am wondering a little bit about align / popup_size.

As far as i can see, the widget does place itself over the full size of his parent. The maximum size of the popup content itself is then just the popup_size. The placement of the popup is then controlled by the align property.

I feel like the popup_size is conceptionally the same as just the max_size_hint we have, and the align_hint is what our align here is. So i would say that those APIs can be replaced with those hint APIs ?

Additionally:

  • The two events do need to be tested
  • The part needs to be tested to be there
herb added a subscriber: herb.Jun 24 2019, 12:08 AM

@bu5hm4n
I think the max_size_hint is different with popup_size.
the max_size in efl_ui_popup means the maximum size that the popup can expand.
If the max_size_hint is used to change the popup size, the popup will not be expandable.
Therefore, I think it's not good to change popup_size to max_size_hint.
In case of the alignment API of popup, It is designed to notify the options to the users.
To use this API makes the code simple and easy.

zmike added a comment.Jun 24 2019, 6:07 AM

I agree that it seems like align could be handled using align_hint.

I'm still not entirely sure (without reading the implementation) what popup_size is supposed to do, but it seems like this runs counter to how EFL handles sizing. All sizing-related things should be handled with size hints.

bu5hm4n updated the task description. (Show Details)Jun 26 2019, 11:37 PM
Diffusion closed subtask T7562: efl.input.interface as Resolved.
zmike added a comment.Aug 12 2019, 8:49 AM

I think we could maybe move timeout to just be an implementation of efl_loop_timer_interval_set?

zmike added a comment.Aug 12 2019, 9:02 AM

Looking at align, this could be moved to the size hint, but the functionality would change pretty radically, particularly in the anchor popup subclass.

Presently, in the base popup class, align is handled without any geometry clamping; this would be trivial to convert.

In the anchor subclass, however, align works very differently. There is a mechanism for setting align 'priorities', and each method of aligning (other than center align) does not permit any 'adjustment'; this means that each attempt to apply an align is all-or-nothing, and if it does not fit the constraints of the layout then that align is discarded and the next one is attempted. The anchor unit test that I've written is based on this premise. If we want to change how this functions, then we would need to rework the mechanics/expectations of aligning (not especially difficult, I've done similar work on a number of previous occasions).

In T7902#139963, @zmike wrote:

I think we could maybe move timeout to just be an implementation of efl_loop_timer_interval_set?

I am thinking this is closer to efl_loop timeout behavior. Maybe returning a future<error> with error set to ETIMEOUT if the popup was closed due to timeout make sense (and zero if it was closed for another reason).

herb added a comment.Aug 12 2019, 9:27 PM

@zmike
Hi, I agree to your opinion, the usage of the anchor popup align is different from base popup.
so I think the align feature in anchor popup should be separated from Efl.Ui.Popup.Align
and make the anchor popup has its own priority align features.

@herb My point was that anchor popup align is handled differently based on parenting, not that the two methods of aligning are incompatible. Currently there is no clamping to the window with the base popup's align, but this is just because there can be no parent besides the window, so the popup is always aligned correctly without any further checks or adjustments.

In particular I am interested in HQ's use cases for the align feature. Is it that application developers actually want to be able to set a priority of alignments, or that they just want the popup to align itself optimally on the screen at all times? If applications never set the align priority, then I don't think having that API is particularly useful, and we could then convert the whole thing to just using align hints instead of a separate align enum.

herb added a comment.Aug 20 2019, 3:15 AM

@zmike
The application developers in tizen need two types of popup such as context style and alert style
In case of the context style like elm_ctxpopup of efl needs priority align feature because the context popup should be shown depending on mouse position and in the screen boundary.
when the context popup is created near the screen boundary, the priority is used to find the optimal position of the context popup which the user wants.
In case of the alert style like elm_popup of efl needs align feature because some developer want the popup to be aligned whenever the screen is resized.
However If there are some ways to set the priority using algin hints or find optimal position, I think we can remove the priority set API :)

herb added a comment.Aug 20 2019, 9:50 PM

@zmike
I talked with woohyun about this refactoring for popup,
we have to finish this task until early september since we will start the release for EflSharp after the middle of september.
please take care of this schedule.

zmike added a comment.Aug 21 2019, 4:15 AM

@herb thanks for the updates on this. I already have nearly everything done at this point, so it should be simple to complete the work by the end of the week at the latest.

zmike updated the task description. (Show Details)Aug 21 2019, 8:53 AM
zmike updated the task description. (Show Details)

Removed action item for tests since this (and subclass functionality) is probably now the most well-tested widget in the entire toolkit.

zmike added a comment.Aug 21 2019, 8:57 AM

This ticket also includes the alert_popup subclass, which implements the button property. This property needs a better name (and possibly reworking?).

If we have eolian part names, we could remove this entirely in favor of content_set+part I think? The enum used here maps to fixed content names in the theme.

herb added a comment.Aug 27 2019, 5:17 AM

@zmike
You mean that making the parts class for the buttons, right?
I also agree with your opinion and I think the parts should implement content and text interfaces for the setting text and icon of buttons

zmike added a comment.Aug 27 2019, 5:23 AM

@herb Do you have time to handle that?

herb added a comment.Aug 27 2019, 5:30 AM

In fact, I don't have enough time to handle this since I have to migrate upstream code to tizen until next wednesday.
and I should make the entire EflSharp UnitTest set until end of september. I think the amount of TC set that I have to make quite large.
so I'm not sure to answer :(

zmike added a comment.Aug 27 2019, 5:33 AM

I will see if I have time...

I raised something similar and got this feedback:

https://phab.enlightenment.org/T7948#137712

zmike moved this task from Backlog to Evaluating on the efl: api board.Aug 29 2019, 7:50 AM

Alright, we're leaving the alert button thing.

The only remaining item here is regarding timeout. I'm not entirely sure what @cedric meant; do we want to do anything with that or leave it as-is? If we leave it, this class (and alert popup) are done.

hmmmmm.... yeah, not very clear. Anyway, I think timeout makes sense here for the user. We could probably implement the same thing by being clever with Futures and save a property, but I don't think it's worth it.

I also agree with timeout, does make sense IMO. the future would need a API that returns it, and we do not have any.

zmike added a comment.Aug 30 2019, 6:19 AM

Are we reasonably sure that timeout won't conflict with anything in the future or should we rename preemptively to popup_timeout now?

Preemptive rename sounds OK.

But I fear where we are headed... we are creating double namespaces, by adding the class name to a lot of properties to avoid conflicts (Efl.Ui.Popup.popup_timeout). Not that we have time to fix it now.

zmike added a comment.Aug 30 2019, 7:14 AM

Is there really an alternative?

How about closing timeout ? It describes what it does and is not prefixed with popup

Yep, closing_timeout has a lower chance of collision.

Alright so this is done.

zmike moved this task from Evaluating to Stabilized on the efl: api board.Aug 30 2019, 11:27 AM
zmike closed subtask T7578: efl.ui.view as Resolved.Sep 26 2019, 8:12 AM