515907 - Support taskbar icon overlay in Windows 7

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Rimas Kudelis [use needinfo to ping]]

Taskbar icons in Windows 7 can have overlays similarly to OS X. I think this would benefit at least Thunderbird and SeaMonkey if there was support for this in Toolkit.

Comment 1

[Rob Arnold [:robarnold]]

Shouldn't be too hard to add - I already did a bunch of the work for taskbar previews.

Comment 2

[Jim Mathies [:jimm]]

This is kind of neat, rob do you want to take this? If not I'd be happy to.

Comment 3

[Rob Arnold [:robarnold]]

Go ahead and take this one if you'd like though it doesn't seem like a priority. You'll probably want to modify the nsIWindowTaskbarPreview interface to have the icon + alt text attributes OR create a new interface (nsIWindowTaskbarIcon?) since this is in a sense separate from the window preview. Either way, I think you should reuse the WindowTaskbarPreview class since you'll need to do the same song & dance with nsWindow creation/destruction and the TaskbarButtonCreated notification for this API.

Comment 4

[David Humphrey (:humph)]

Tim is going to give this one a shot.

Comment 5

[Tim Miller (UCOSP student) [:tmiller]]

After searching through the interface files associated with the windows taskbar and looking at the microsoft ITaskbarList4 interface, I've decided to create a new interface called nsITaskbarIcon (not nsITaskbarWindowIcon or nsITaskbarTabIcon because there is only only one icon, whereas the preview can be for either a tab or a window).  I am using the TaskbarPreview class as a guide.

Comment 6

[Rob Arnold [:robarnold]]

I did some experimenting with the behavior. There's a sample app in the sdk (TaskbarPeripheralStatus) that you can play with. The behavior seems to be:
small icons -> no overlay
large icons, ungrouped -> each window shows its overlay
large icons, grouped (default) -> whichever window had its overlay icon set to an icon (as opposed to NULL) last is shown on the taskbar button

With this behavior, I think we should set the icon on a per window basis and make it clear how merging works.

I like the idea of a separate interface but for the implementation, reusing WindowTaskbarPreview is probably a good idea.

Comment 7

[Rain [:rain1] (inactive)]

(In reply to comment #6)
> I like the idea of a separate interface but for the implementation, reusing
> WindowTaskbarPreview is probably a good idea.

nsITaskbarProgress does the same thing, so you could use it as a guide.

Comment 8

[Tim Miller (UCOSP student) [:tmiller]]

I ended up putting the interface declaration in nsITaskbarProgress as both are part of the same ITaskbarList4 interface in the Windows API.

Comment 9

[Tim Miller (UCOSP student) [:tmiller]]

Attachment: Adds the setOverlayIcon() interface to nsITaskBarProgress.idl and it's definition to TaskbarWindowPreview.cpp

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 428968 [details] [diff] [review]
Adds the setOverlayIcon() interface to nsITaskBarProgress.idl and it's definition to TaskbarWindowPreview.cpp

Hi Tim,

sorry for the late review, but here you go.

>+++ b/widget/public/nsITaskbarProgress.idl
>@@ -34,23 +35,25 @@
>-[scriptable, uuid(23ac257d-ef3c-4033-b424-be7fef91a86c)]
>+[scriptable, uuid(B621629C-223A-11DF-901E-FD3156D89593)]

It looks like we prefer to use lowercase letters in our uuids.

>@@ -82,9 +85,38 @@ interface nsITaskbarProgress : nsISuppor
>+   * @param aStatusDescription The alt text version of the information 
>+   *                 conveyed by the overlay, for accessibility 
>+   *               purposes.    
>+   *
>+   * NOTES:
>+   *     The icon should be a small icon, measuring 16x16 pixels at 96 dpi.
>+   *     If an overlay icon is already applied to the taskbar button, that 
>+   *     existing overlay is replaced.
>+   *     aStatusIcon can be NULL. How a NULL value is handled depends on 
>+   *    whether the taskbar button represents a single window or a group of
>+   *     windows: 
>+   *       If the taskbar button represents a single window, the overlay icon
>+   *        is removed from the display.
>+   *       If the taskbar button represents a group of windows and a previous 
>+   *        overlay is still available (received earlier than the current 
>+   *        overlay, but not yet freed by a NULL value), then that previous
>+   *        overlay is displayed in place of the current overlay.
>+   *    Windows makes and uses its own copy of the icon when a new icon is
>+   *     set, so it is safe to free aStatusIcon after a call to this method.
>+   */
>+  void setOverlayIcon(in imgIContainer statusIcon, 
>+                      in nsString statusDescription);
>+  

There are a bunch of tabs here that should be spaces.
And a bunch of trailing spaces which should be removed.

>+++ b/widget/src/windows/TaskbarWindowPreview.cpp
>@@ -301,13 +302,31 @@ TaskbarWindowPreview::UpdateButton(PRUin
>+NS_IMETHODIMP
>+TaskbarWindowPreview::SetOverlayIcon(imgIContainer *aStatusIcon, 
>+                                   nsString *aStatusDescription){

So, this didn't line up for me, and it turned out to be because of the
tabs, which led me to look at the previous file, which also had tabs in it.
So, same as above, please convert the tabs to spaces, and remove the
trailing spaces and the ^Ms.

And, we usually put a space after the ) and before the {.

>+if (!CanMakeTaskbarCalls())

This line should be indented by two spaces.

>+    return NS_OK; 
>+  HICON *hIcon = NULL;
>+  if(aStatusIcon){

Spaces before and after the ()s, please.

>+    nsWindowGfx::CreateIcon(aStatusIcon, false, 0, 0, hIcon);
>+  }
>+  LPCWSTR aString = NULL;
>+  if(aStatusDescription){

Ditto.

>+     aString = (LPCWSTR)aStatusDescription->get();

Space after the ), please.

>+  }
>+  HRESULT hr = mTaskbar->SetOverlayIcon(mWnd, *hIcon, aString);
>+
>+  return SUCCEEDED(hr) ? NS_OK : NS_ERROR_FAILURE;
>+}

So, once you've fixed those, you should ask some of the people listed at
http://www.mozilla.org/about/owners.html#widget-windows for another couple
of reviews (because my review doesn't count, and you need two), and for a super-review.  The people who reviewed the previous
version of this code were: r=jimm, r=robarnold, sr=roc.  So they would seem
to be reasonable choices.

Later,
Blake.

Comment 11

[Tim Miller (UCOSP student) [:tmiller]]

Attachment: Fixed whitespace problems from the previous attachment.

Comment 12

[Jim Mathies [:jimm]]

+   * @param aStatusIcon The handle to the overlay icon.

"..overlay icon or null to clear a previously set icon."

+   *   The icon should be a small icon, measuring 16x16 pixels at 96 dpi.

Can/should we enforce that when we receive the call? What happens if someone passes in a non-conforming icon?

+  LPCWSTR aString = NULL;

please use PRUnichar.

Comment 13

[Rob Arnold [:robarnold]]

Comment on attachment 434920 [details] [diff] [review]
Fixed whitespace problems from the previous attachment.

>diff --git a/widget/public/nsITaskbarProgress.idl b/widget/public/nsITaskbarProgress.idl
>--- a/widget/public/nsITaskbarProgress.idl
>+++ b/widget/public/nsITaskbarProgress.idl
>+interface imgIContainer;
>+interface nsString;

I think this should be DOMString. You only need to change the IDL however.

Also, I think we want a separate interface for this - the progress meter and overlay icons are not related conceptually, only their implementation on Windows 7 is. The TaskbarWindowPreview class should implement the new interface so you don't need to move your C++ code.

>+  /**
>+   * Sets the overlay icon and it's corresponding alt text.
>+   *
>+   * @param aStatusIcon The handle to the overlay icon.
>+   *
>+   * @param aStatusDescription The alt text version of the information
>+   *                           conveyed by the overlay, for accessibility
>+   *                           purposes.
>+   *
>+   * NOTES:
>+   *   The icon should be a small icon, measuring 16x16 pixels at 96 dpi.
>+   *   If an overlay icon is already applied to the taskbar button, that
>+   *   existing overlay is replaced.
>+   *   statusIcon can be NULL. How a NULL value is handled depends on
>+   *   whether the taskbar button represents a single window or a group of
>+   *   windows:
>+   *     If the taskbar button represents a single window, the overlay icon
>+   *       is removed from the display.
>+   *     If the taskbar button represents a group of windows and a previous
>+   *       overlay is still available (received earlier than the current
>+   *       overlay, but not yet freed by a NULL value), then that previous
>+   *       overlay is displayed in place of the current overlay.
>+   *   Windows makes and uses its own copy of the icon when a new icon is
>+   *   set, so it is safe to free statusIcon after a call to this method.

imgIContainer is ref counted/garbage collected so no need to give this warning I think.

>+NS_IMETHODIMP
>+TaskbarWindowPreview::SetOverlayIcon(imgIContainer *aStatusIcon,
>+                                     nsString *aStatusDescription){

This should be nsAString & instead of nsString.

>+  if (!CanMakeTaskbarCalls())
>+    return NS_OK;

This isn't the right behavior. If we can't make the call to the taskbar now, we need to make it later once we can. The rest of the taskbar code tends to store the parameters as members of the class so that once we can call the taskbar, we can make the calls (see TaskbarWindowPreview::UpdateTaskbarProperties). Note that there's no way to report an error for these delayed calls. In this case, I think two members, nsString mStatusDescription; and HICON mStatusIcon; would be sufficient.

>+  HICON *hIcon = NULL;
I think this needs to be an HICON instead of HICON *
>+  if (aStatusIcon) {
>+    nsWindowGfx::CreateIcon(aStatusIcon, false, 0, 0, hIcon);

Then pass &hIcon instead of hIcon

>+  HRESULT hr = mTaskbar->SetOverlayIcon(mWnd, *hIcon, aString);

This bit will need to move down into a new method, TaskbarWindowPreview::UpdateTaskbarIcon, which should be called in TaskbarWindowPreview::UpdateTaskbarProperties. If we can make taskbar calls here, we should call UpdateTaskbarIcon now.

Comment 14

[Joe Drew (not getting mail)]

Won't block on this, but OMGwant.

Comment 15

[Jim Mathies [:jimm]]

Comment on attachment 434920 [details] [diff] [review]
Fixed whitespace problems from the previous attachment.

needs comments addressed.

Comment 16

[Rob Arnold [:robarnold]]

Tim: any update on this? Are you still working on it?

Comment 17

[Rain [:rain1] (inactive)]

Attachment: revised and updated patch v3

All right, so this aims to address all the issues Rob mentioned. Specifically
- I've moved the function out into a separate interface. The current name's pretty crappy, so suggestions welcome :)
- The code now behaves like all the other code we have here and if it can't make calls now, makes calls whenever it can.
- Fixed the hIcon.

I also added a check making sure the icon's sized 16x16.

Questions:
- Why should the status icon description a DOMString and not just an AString?
- Am I doing the right thing by calling UpdateTaskbarProperties() in the window hook?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Are animated images supported?

Why are we removing the progress API?

Comment 19

[Rain [:rain1] (inactive)]

> Are animated images supported?

No, just static icons. http://msdn.microsoft.com/en-us/library/dd391696(v=vs.85).aspx says that overlay icons shouldn't be animated.

> Why are we removing the progress API?

Heh, we aren't, that was just an artifact of me using hg cp to create a new file. If you look at the raw patch you'll see a copy from/copy to instruction. Sorry for the confusion :)

Comment 20

[Rain [:rain1] (inactive)]

(In reply to Siddharth Agarwal [:sid0] from comment #19)
> > Are animated images supported?
> 
> No, just static icons.

Actually I take that back and replace it with "I don't know". Depends on what nsWindowGfx::CreateIcon does I guess.

Comment 21

[James Ross]

Comment on attachment 552517 [details] [diff] [review]
revised and updated patch v3

Review of attachment 552517 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/src/windows/TaskbarWindowPreview.cpp
@@ +200,5 @@
>  
> +NS_IMETHODIMP
> +TaskbarWindowPreview::SetOverlayIcon(imgIContainer* aStatusIcon,
> +                                     const nsAString& aStatusDescription) {
> +  // The API mandates that the icon be 16x16.

The documentation says "This should be a small icon, measuring 16x16 pixels at 96 dpi", which isn't quite the same as mandating 16x16 always. I suspect it wants a GetSystemMetrics(SM_CXSMICON) by GetSystemMetrics(SM_CYSMICON) icon, which probably means this API should be scaling the input image if necessary too.

Comment 22

[Rain [:rain1] (inactive)]

(In reply to James Ross from comment #21)
> The documentation says "This should be a small icon, measuring 16x16 pixels
> at 96 dpi", which isn't quite the same as mandating 16x16 always. I suspect
> it wants a GetSystemMetrics(SM_CXSMICON) by GetSystemMetrics(SM_CYSMICON)
> icon, which probably means this API should be scaling the input image if
> necessary too.

Interesting point -- by "this API" do you mean us or the Windows API?

Comment 23

[James Ross]

"This API" was refering to TaskbarWindowPreview::SetOverlayIcon which, if I understand correctly, chrome JS code in various windows is likely to be calling more or less directly to set overlays of their choosing.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Siddharth Agarwal [:sid0] from comment #20)
> (In reply to Siddharth Agarwal [:sid0] from comment #19)
> > > Are animated images supported?
> > 
> > No, just static icons.
> 
> Actually I take that back and replace it with "I don't know". Depends on
> what nsWindowGfx::CreateIcon does I guess.

Can you just document (and maybe assert) that animated images must not be used here?

Comment 25

[Rob Arnold [:robarnold]]

Comment on attachment 552517 [details] [diff] [review]
revised and updated patch v3

>+  HICON hIcon = NULL;
>+  if (aStatusIcon) {
>+    rv = nsWindowGfx::CreateIcon(aStatusIcon, false, 0, 0, &hIcon);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+  mOverlayIcon = hIcon;

Do you need to destroy mOverlayIcon if it's not null prior to this assignment?

Comment 26

[Rain [:rain1] (inactive)]

(In reply to Rob Arnold [:robarnold] from comment #25)
> Comment on attachment 552517 [details] [diff] [review]
> revised and updated patch v3
> 
> >+  HICON hIcon = NULL;
> >+  if (aStatusIcon) {
> >+    rv = nsWindowGfx::CreateIcon(aStatusIcon, false, 0, 0, &hIcon);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+  }
> >+  mOverlayIcon = hIcon;
> 
> Do you need to destroy mOverlayIcon if it's not null prior to this
> assignment?

Good catch! From http://msdn.microsoft.com/en-us/library/dd391696:

"It is the responsibility of the calling application to free hIcon when it is no longer needed. This can generally be done after you call SetOverlayIcon because the taskbar makes and uses its own copy of the icon."

Since we store our own copy as well, we should destroy it before reassigning.

Comment 27

[Rain [:rain1] (inactive)]

Attachment: patch v4

- Added a check for animated icons.
- Added a DestroyIcon call and made sure mOverlayIcon is set to NULL in the beginning.

roc: do you think the scaling Silver proposed is necessary?

Comment 28

[Rob Arnold [:robarnold]]

Comment on attachment 553118 [details] [diff] [review]
patch v4

Review of attachment 553118 [details] [diff] [review]:
-----------------------------------------------------------------

r=me w/this change

::: widget/src/windows/TaskbarWindowPreview.cpp
@@ +224,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  if (mOverlayIcon)
> +    ::DestroyIcon(mOverlayIcon);

This needs to happen in the destructor too.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Siddharth Agarwal [:sid0] from comment #27)
> Created attachment 553118 [details] [diff] [review]
> patch v4
> 
> - Added a check for animated icons.
> - Added a DestroyIcon call and made sure mOverlayIcon is set to NULL in the
> beginning.
> 
> roc: do you think the scaling Silver proposed is necessary?

Probably. You should probably test by setting the "font DPI" to something other than 96.

Comment 30

[Rain [:rain1] (inactive)]

I put up a small extension that'll make this patch easier to test at https://github.com/sid0/overlay-extension. I'm considering putting it up on AMO...

Comment 31

[Jim Mathies [:jimm]]

Comment on attachment 553118 [details] [diff] [review]
patch v4

(In reply to Siddharth Agarwal [:sid0] from comment #26)
> (In reply to Rob Arnold [:robarnold] from comment #25)
> > Comment on attachment 552517 [details] [diff] [review]
> > revised and updated patch v3
> > 
> > >+  HICON hIcon = NULL;
> > >+  if (aStatusIcon) {
> > >+    rv = nsWindowGfx::CreateIcon(aStatusIcon, false, 0, 0, &hIcon);
> > >+    NS_ENSURE_SUCCESS(rv, rv);
> > >+  }
> > >+  mOverlayIcon = hIcon;
> > 
> > Do you need to destroy mOverlayIcon if it's not null prior to this
> > assignment?
> 
> Good catch! From http://msdn.microsoft.com/en-us/library/dd391696:
> 
> "It is the responsibility of the calling application to free hIcon when it
> is no longer needed. This can generally be done after you call
> SetOverlayIcon because the taskbar makes and uses its own copy of the icon."
> 
> Since we store our own copy as well, we should destroy it before reassigning.

Why don't we just call DestroyIcon after the call to mTaskbar->SetOverlayIcon in TaskbarWindowPreview::UpdateOverlayIcon()? Seems like we don't need to keep this around if I'm reading comments right. We'd still need do the other checks of course, but might as well destroy it as soon as we can.

Comment 32

[Rob Arnold [:robarnold]]

(In reply to Jim Mathies [:jimm] from comment #31)
> Why don't we just call DestroyIcon after the call to
> mTaskbar->SetOverlayIcon in TaskbarWindowPreview::UpdateOverlayIcon()? Seems
> like we don't need to keep this around if I'm reading comments right. We'd
> still need do the other checks of course, but might as well destroy it as
> soon as we can.

We might get two calls to SetOverlayIcon before we get the message that allows us to call mTaskbar->SetOverlayIcon. If we don't check then, we'll leak the first icon.

Comment 33

[Rain [:rain1] (inactive)]

Attachment: patch v5

I found a couple boo-boos while testing my patch out using the extension. This patch has them fixed. No scaling yet though.

Comment 34

[Rain [:rain1] (inactive)]

I've put the extension up at http://people.mozilla.org/~sagarwal/overlay-extension/overlay-extension-0.001.xpi -- the latest version should always be at http://people.mozilla.org/~sagarwal/overlay-extension/overlay-extension-latest.xpi. I've also fired off a Firefox try build with the patch.

Comment 35

[Rain [:rain1] (inactive)]

Try build for v5: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c2c2ab54a7b9/try-win32/firefox-8.0a1.en-US.win32.zip

Comment 36

[Benoit Girard (:BenWa)]

The OS X dock has a similar, feature although it's not strictly an overlay. Should we file a bug to get the analogous implemented in the cocoa widget?

Comment 37

[Rain [:rain1] (inactive)]

(In reply to Benoit Girard (:BenWa) from comment #36)
> The OS X dock has a similar, feature although it's not strictly an overlay.
> Should we file a bug to get the analogous implemented in the cocoa widget?

Probably? We Thunderbird folks have custom code for it.

Comment 38

[Rain [:rain1] (inactive)]

Attachment: patch v6

Now with scaling! I've added the code to nsWindowGfx.

Jim: I'm asking for re-review because the patch is substantially different.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 560690 [details] [diff] [review]
patch v6

Review of attachment 560690 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/public/nsITaskbarProgress.idl
@@ +67,5 @@
> +   *       is removed from the display.
> +   *     If the taskbar button represents a group of windows and a previous
> +   *       overlay is still available (received earlier than the current
> +   *       overlay, but not yet freed by a NULL value), then that previous
> +   *       overlay is displayed in place of the current overlay.

Does this mean that if you do N setOverlayIcon calls on a group of windows, to remove all the overlay icons you need to call setOverlayIcon again with NULL statusIcon, N times?

Comment 40

[Rain [:rain1] (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39)
> 
> Does this mean that if you do N setOverlayIcon calls on a group of windows,
> to remove all the overlay icons you need to call setOverlayIcon again with
> NULL statusIcon, N times?

Yes, that's right. Our taskbar progress code works the same way.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 560690 [details] [diff] [review]
patch v6

Review of attachment 560690 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, so setOverlayIcon acts like a stack. Maybe it would be clearer to have two methods, pushOverlayIcon and popOverlayIcon, to make this very clear?

Comment 42

[Rain [:rain1] (inactive)]

Well, it's kind of a stack, but a setOverlayIcon on the same window will cause the previous value to be cleared from it. I think making it behave almost but not exactly like a stack will be problematic.

So, something like
a.setOverlayIcon(foo);
b.setOverlayIcon(bar);
a.setOverlayIcon(baz);
c.setOverlayIcon(quux);

followed by

c.setOverlayIcon(null);
a.setOverlayIcon(null);
b.setOverlayIcon(null);

will clear the overlay icon completely.

Also our current API is per-window, and the stack implementation would need to be per-window group I think. I'm not really sure how this would be implemented, especially since some people don't use window groups on Win7.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It seems like you might need different APIs for windows and for window groups? The current API is confusing.

Comment 44

[Rain [:rain1] (inactive)]

The Windows API lets you set an overlay icon for windows, then Windows itself figures out how to do it for window groups.

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Then I would just omit specifying what this does for window groups. Just say that this API sets the current icon for the window. Maybe as an extra informational note you could explain how Windows computes the overlay for a group of windows based on the icons that have been set for the windows in the group.

Comment 46

[Rain [:rain1] (inactive)]

Sure, that sounds like a good idea.

Comment 47

[Rain [:rain1] (inactive)]

Attachment: patch v7

All right, I've updated the comment to make it clear that Windows manages taskbar buttons for window groups.

Comment 48

[Rain [:rain1] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cd3dc884d1c

Comment 49

[Michael Wu [:mwu]]

https://hg.mozilla.org/mozilla-central/rev/9cd3dc884d1c

Comment 50

[WildcatRay]

It appears that the landing of this bug affects Win7 taskbar previews on systems using Intel (HD) Graphics. The content of each tab does not display as expected. What info can or should I provide in, I assume, the new bug you would like filed? Thanks.

Comment 51

[Guillermo Moya (MetalS)]

This happens to me as well on a NVIDIA GT 540M and other users have reported this to happen in other GPU such as the ATI. This is a regression.
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:10.0a1) Gecko/20110930 Firefox/10.0a1

Comment 52

[Rain [:rain1] (inactive)]

sheppy: https://github.com/sid0/overlay-extension should help you out :)

Comment 53

[enzomas]

Looks like overlay icon feature does not work anymore, with FF 38 and TB 38.

With thunderbird, Taskbar Windows Taskbar Unread Badge add-on (https://addons.mozilla.org/fr/thunderbird/addon/unread-badge/) throws NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsITaskbarOverlayIconController.setOverlayIcon]

Same exception when trying Sid's test extension - see comment 52 - in firefox.

Comment 54

[Brandon Streiff [:bstreiff]]

(In reply to enzomas from comment #53)
> Looks like overlay icon feature does not work anymore, with FF 38 and TB 38.

I've filed that as bug #1176448.

Comment 55

[enzomas]

Bug #1176448 remained unconfirmed for 2 months.
The fact is that icon overlay support is broken now : we should reopen ?