Giannis Adamopoulos

  • More
  • CR-122
  • finished reviewing
What you mean is aggregation but we tend to avoid it. it makes things confusing some times and on top of that our ATL doesn't support aggregation yet.

What you mean is aggregation but we tend to avoid it. it makes things confusing some times and on top of that our ATL doesn't support aggregation yet.

That was my question as well. Why wasn't id-1 appropriate as in lpVerb?

That was my question as well. Why wasn't id-1 appropriate as in lpVerb?

So productive. You could instead ask wtf USA stands for in this function.

So productive. You could instead ask wtf USA stands for in this function.

This is a personal opinion but since you have two big ors it would be more readable if each of them was its own if.

This is a personal opinion but since you have two big ors it would be more readable if each of them was its own if.

You don't need to keep m_hInstance. You also don't seem to initialize it anywhere or use it.

You don't need to keep m_hInstance. You also don't seem to initialize it anywhere or use it.

You don't need to keep either of these.

You don't need to keep either of these.

We created CQuickLaunchBand which can be registered like a typical shell extension, so that we can develop and test CISFBand against windows explorer. (Which keeps revealing bugs in our explorer).

We created CQuickLaunchBand which can be registered like a typical shell extension, so that we can develop and test CISFBand against windows explorer. (Which keeps revealing bugs in our explorer).

My question is what are you trying to achieve with this code.

My question is what are you trying to achieve with this code.

It should probably return an error code.

It should probably return an error code.

See the documentation of WM_RBUTTONUP to see what it expects you to return.

See the documentation of WM_RBUTTONUP to see what it expects you to return.

The same question is valid for all classes that inherit CWindow and implement a window and needs some thinking,

The same question is valid for all classes that inherit CWindow and implement a window and needs some thinking,

But it conflicts with some other headers (atl I think).

But it conflicts with some other headers (atl I think).

Keep in mind that your CISFBand already is a CWindowImpl. Which means that you could just get rid of m_hWndTb and use the m_hWnd of the CWindowImpl.

Keep in mind that your CISFBand already is a CWindowImpl. Which means that you could just get rid of m_hWndTb and use the m_hWnd of the CWindowImpl.

There is no such class (and hence guid) in windows. Quicklaunch and Desktop folders can be added in the taskbar in windows but they are not standalone classses. When you press to add either of them...

There is no such class (and hence guid) in windows. Quicklaunch and Desktop folders can be added in the taskbar in windows but they are not standalone classses. When you press to add either of them , a CISFBand is created and initialized appropriately.

Ok. Took me 5 minutes to see how it should work. There should indeed be no subclassing. The toolbar will send the WM_COMMAND message which will reach the tray window. Based on its LPARAM which is t...

Ok. Took me 5 minutes to see how it should work. There should indeed be no subclassing. The toolbar will send the WM_COMMAND message which will reach the tray window. Based on its LPARAM which is the window the created the message, it will be forwarded to CBandSite::OnWinEvent which will in turn forward the message to the right band. So what should change in the CISFBand is that CISFBand::OnWinEvent should handle WM_COMMMAND messages.(And of course don't subclass the toolbar).

EDIT: I just realized that fixing this will also fix pressing an item in the chevron menu (which only works in windows explorer for now).

This will be more complex actually. CISFBand can get the name of the folder it is showing but I also thing that one of the two interfaces exclusive to the CISFBand allow an arbitrary string to be s...

This will be more complex actually. CISFBand can get the name of the folder it is showing but I also thing that one of the two interfaces exclusive to the CISFBand allow an arbitrary string to be set. So let's keep it like it is now until it is implemented correctly.

WM_NOTIFY are sent to the parent and he initialy did this. However this gave me another idea. Perhaps the parent (the CBandSite) should forward the WM_NOTIFY to OnWinEvent method and no subclassing...

WM_NOTIFY are sent to the parent and he initialy did this. However this gave me another idea. Perhaps the parent (the CBandSite) should forward the WM_NOTIFY to OnWinEvent method and no subclassing is needed. I'll have a look at it.

That's strange. Does it really work? Try using debug channels the same way NtUser does. (Use DBG_DEFAULT_CHANNEL macro). Just note that the names of the channels are predefined.

That's strange. Does it really work? Try using debug channels the same way NtUser does. (Use DBG_DEFAULT_CHANNEL macro). Just note that the names of the channels are predefined.

I think we could just pass the parameters in the stack, no?

I think we could just pass the parameters in the stack, no?

Yeah, it seems it also needs refcounting. windows seem to do something similar: https://www.reactos.org/wiki/Techwiki:Win32k/FILEVIEW

Yeah, it seems it also needs refcounting. windows seem to do something similar: https://www.reactos.org/wiki/Techwiki:Win32k/FILEVIEW

A FontMemEntry is about a font that is not backed by a file?

A FontMemEntry is about a font that is not backed by a file?

Katayama, this change set is about cleaning up. Let's focus on that.

Katayama, this change set is about cleaning up. Let's focus on that.

DPRINT1 for an error is fine.

DPRINT1 for an error is fine.

perhaps we need to assert here?

perhaps we need to assert here?

I tend to avoid this style. Storing the value before testing it can make it easier to debug

I tend to avoid this style. Storing the value before testing it can make it easier to debug

Isn't it better to check the return value?

Isn't it better to check the return value?

You didn't find the algorithm to compute the hash, did you?

You didn't find the algorithm to compute the hash, did you?

I added a second iteration of the patch. Feel free to have a look and review. Also make sure that you select to see the second iteration (either compared to the first iteration or base).

I added a second iteration of the patch. Feel free to have a look and review. Also make sure that you select to see the second iteration (either compared to the first iteration or base).