Page MenuHomePhabricator

WindowManager interface for opening is not user-friendly
Closed, ResolvedPublic

Description

I've seen the open() -> then/opening -> then/opened pattern one too many times. I've tried numerous times, but I still don't understand what it's for. I see other developers using it also struggling with it.

Thanks to our great documentation now, it is quite easy to spot the problem in our API. See the below example.

windowManager.openWindow( messageDialog, {
  title: 'Hello',
  message: 'How are you?',
  actions: [
    { label: 'Good', action: 'good' },
    { label: 'Bad', action: 'bad' },
    { label: 'Nevermind' }
  ],
} ).then( function ( opening ) {
  opening.then( function ( opened ) {
    opened.then( function ( data ) {
      ... data ..
    } );
  } );
} );

// Or:

windowManager.openWindow( messageDialog, {
  title: 'Hello',
  message: 'How are you?',
  actions: [
    { label: 'Good', action: 'good' },
    { label: 'Bad', action: 'bad' },
    { label: 'Nevermind' }
  ],
} ).then( function ( opening ) {
  return opening;
} ).then( function ( opened ) {
  return opened;
} ).then( function ( data ) {
  ... data ..
} );

The middle two steps don't do anything. And there's no clear reason why they are required.

This task should be resolved by either:

  • Changing our code to not have this extra step.
  • Or; if they exist for a reason and are important enough to justify overloading the promise (as opposed to an opt-in event handler), we should provide a shortcut.
  • And/or; Update our documentation to use the better pattern.

Event Timeline

Krinkle created this task.Apr 13 2015, 6:12 PM
Krinkle raised the priority of this task from to Needs Triage.
Krinkle updated the task description. (Show Details)
Krinkle added projects: OOUI, Technical-Debt.
Krinkle added a subscriber: Krinkle.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 13 2015, 6:12 PM
Jdforrester-WMF triaged this task as Lowest priority.Mar 22 2016, 6:32 PM
Jdforrester-WMF set Security to None.
Krinkle removed a subscriber: Krinkle.Mar 22 2016, 7:40 PM

Change 350789 had a related patch set uploaded (by Bartosz Dziewoński; owner: Krinkle):
[oojs/ui@master] WindowManager: Add WindowInstance - a Promise-based lifecycle object

https://gerrit.wikimedia.org/r/350789

There has been some discussion about the new approach submitted above on T163510, wherein we discovered that the current weird interface actually doesn't work with jQuery 3.

Change 350789 merged by jenkins-bot:
[oojs/ui@master] WindowManager: Add WindowInstance - a Promise-based lifecycle object

https://gerrit.wikimedia.org/r/350789

Change 356061 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] WindowManager: Deprecate using the return value of openWindow/closeWindow as a promise

https://gerrit.wikimedia.org/r/356061

Volker_E moved this task from Backlog to Reviewing on the OOUI board.May 29 2017, 10:29 PM

Change 356061 merged by jenkins-bot:
[oojs/ui@master] WindowManager: Deprecate using returns of openWindow/closeWindow as promises

https://gerrit.wikimedia.org/r/356061

matmarex closed this task as Resolved.Jun 1 2017, 4:47 PM
matmarex assigned this task to Krinkle.

So, the new usage is:

instance = windowManager.openWindow( ... );
instance.opening.then( function () {
  // Code here runs after the window has started opening
} );
instance.opened.then( function () {
  // Code here runs after the window is opened
} );
instance.closing.then( function () {
  // Code here runs after the window has started closing
} );
instance.closed.then( function ( data ) {
  // Code here runs after the window is closed
} );

Of course, all of the steps are optional. If you just want to open window and wait for it to be closed, use:

windowManager.openWindow( ... ).closed.then( function ( data ) {
  // Code here runs after the window is closed
} );

I'll be updating Wikimedia-related code to use the new convention (T166729). The old convention is still working, but generates deprecation warnings.

In the meantime, here are simple practical examples: https://phabricator.wikimedia.org/diffusion/GOJU/browse/master/src/windows.js (this is the implementation of OO.ui.alert(), .confirm() and .prompt()).

Jdforrester-WMF moved this task from Reviewing to OOjs-UI-0.22.1 on the OOUI board.Jun 1 2017, 7:40 PM
Jdforrester-WMF edited projects, added OOUI (OOjs-UI-0.22.1); removed OOUI.