Re: [PATCH 2/5] shell32: Stub out FolderItems.

[Sebastian Lackner]

On 04.07.2016 07:25, Alex Henrie wrote:
> Cc: Christian Costa 
> Cc: Sebastian Lackner
>
> Wine Staging has included a similar patch since 1.7.51, however this
> patch is not based on theirs.

Actually the patch looks very similar, but this is probably caused by
the fact that you both wrote it based on existing code.
> FolderItems2 worked in Windows 2000, but it was removed when
> FolderItems3 was introduced in Windows XP. However, it doesn't hurt us
> to provide this interface anyway for any old applications that might
> want it.
> Signed-off-by: Alex Henrie
>
> dlls/shell32/shell32_main.h  |   1 +
>  dlls/shell32/shelldispatch.c | 207 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 206 insertions(+), 2 deletions(-)

Some of the variable names (for example ppTInfo, rgszNames, rgDispId) sound
a bit odd. Usually variable names without ugly prefix are preferred for new
code (except we want to keep it for compatibility with old code here?).
> +static HRESULT WINAPI FolderItemsImpl_Item(FolderItems3 *iface, VARIANT index, FolderItem **ppid)
> +{
> - FIXME("(%p,%p)\n", iface, ppid);

You forgot to trace the index parameter here.
> -
> - return E_NOTIMPL;
>
> +}

[...]
> @@ -1093,8 +1297,7 @@ static HRESULT WINAPI FolderImpl_Items(Folder3 *iface, FolderItems **ppid)
>  {
>      FIXME("(%p,%p)\n", iface, ppid);

You forgot to change the FIXME to a TRACE.
> - *ppid = NULL;
> - return E_NOTIMPL;
> - return FolderItems_Constructor(ppid);
>
> }
>
> static HRESULT WINAPI FolderImpl_ParseName(Folder3 *iface, BSTR name, FolderItem **item)

[PATCH 2/5] shell32: Stub out FolderItems.

[Alex Henrie]

2016-07-04 5:09 GMT-06:00 Sebastian Lackner :
> On 04.07.2016 07:25, Alex Henrie wrote:
> > [...]
>
> Some of the variable names (for example ppTInfo, rgszNames, rgDispId) sound
> a bit odd. Usually variable names without ugly prefix are preferred for new
> code (except we want to keep it for compatibility with old code here?).

I was trying to make the new code match the surrounding code. I don't
think the variable names matter very much here, but I'll change them
if you feel strongly about it.
> > +static HRESULT WINAPI FolderItemsImpl_Item(FolderItems3 *iface, VARIANT index, FolderItem **ppid)
> > +{
> > - FIXME("(%p,%p)\n", iface, ppid);
>
> You forgot to trace the index parameter here.

Good point. I'll fix that and resubmit the series (although hopefully
Alexandre will have taken patch 1 without changes).
> > @@ -1093,8 +1297,7 @@ static HRESULT WINAPI FolderImpl_Items(Folder3 *iface, FolderItems **ppid)
> >  {
> >      FIXME("(%p,%p)\n", iface, ppid);
>
> You forgot to change the FIXME to a TRACE.

This is actually intentional, because the patch makes this function
return a list of items, but the list is empty. I'm going to change the
FIXME to a TRACE in the patch that populates the list:
https://github.com/alexhenrie/wine/commits/master

Thanks for the feedback.

-Alex

[PATCH 2/5] shell32: Stub out FolderItems.

[Alex Henrie]

2016-07-04 8:37 GMT-06:00 Alex Henrie :
> 2016-07-04 5:09 GMT-06:00 Sebastian Lackner :
> > On 04.07.2016 07:25, Alex Henrie wrote:
> > > [...]
> >
> > Some of the variable names (for example ppTInfo, rgszNames, rgDispId) sound
> > a bit odd. Usually variable names without ugly prefix are preferred for new
> > code (except we want to keep it for compatibility with old code here?).
>
> I was trying to make the new code match the surrounding code. I don't
> think the variable names matter very much here, but I'll change them
> if you feel strongly about it.
> > > +static HRESULT WINAPI FolderItemsImpl_Item(FolderItems3 *iface, VARIANT index, FolderItem **ppid)
> > > +{
> > > - FIXME("(%p,%p)\n", iface, ppid);
> >
> > You forgot to trace the index parameter here.
>
> Good point. I'll fix that and resubmit the series (although hopefully
> Alexandre will have taken patch 1 without changes).
> > > @@ -1093,8 +1297,7 @@ static HRESULT WINAPI FolderImpl_Items(Folder3 *iface, FolderItems **ppid)
> > >  {
> > >      FIXME("(%p,%p)\n", iface, ppid);
> >
> > You forgot to change the FIXME to a TRACE.
>
> This is actually intentional, because the patch makes this function
> return a list of items, but the list is empty. I'm going to change the
> FIXME to a TRACE in the patch that populates the list:
> https://github.com/alexhenrie/wine/commits/master
>
> Thanks for the feedback.
>
> -Alex

I grepped the code and the names ppTInfo, rgszNames, and rgDispId are
used pretty consistently throughout Wine. I think that unless we're
going to refactor the names across the entire codebase, we might as
well keep them.

So, the only change I've made between v1 and v2 of this patchset is
adding the missing parameter to FolderItemsImpl_Item's FIXME
statement.

-Alex

[PATCH 2/5] shell32: Stub out FolderItems.

[Michael Stefaniuc]

Alex,

On 07/05/2016 07:50 AM, Alex Henrie wrote:
> I grepped the code and the names ppTInfo, rgszNames, and rgDispId are
> used pretty consistently throughout Wine. I think that unless we're
> going to refactor the names across the entire codebase, we might as
> well keep them.

you miss the point. That is *old* code when Alexandre was "still young
and needed the money"...

The rules have changed and new code should be clean even if it is
surrounded by old ugly code.

bye
    michael