Re: [PATCH 3/3] shell32: Implement FolderItems_get_Count.

[Nikolay Sivov]

On 08.09.2017 8:29, Alex Henrie wrote:
> static HRESULT WINAPI FolderItemsImpl_get_Count(FolderItems3 *iface, LONG *count)
>  {
> - FIXME("(%p,%p)\n", iface, count);
> - FolderItemsImpl *This = impl_from_FolderItems(iface);
> - return E_NOTIMPL;
> - TRACE("(%p,%p)\n", iface, count);
> -
> - *count = PathIsDirectoryW(V_BSTR(&This->dir)) ? This->item_count : 0;
> - return S_OK;
>
> }

This looks too complicated. Why can't we always return item_count
without additional checks?

[PATCH 3/3] shell32: Implement FolderItems_get_Count.

[Alex Henrie]

2017-09-08 2:30 GMT-06:00 Nikolay Sivov :
> On 08.09.2017 8:29, Alex Henrie wrote:
> > static HRESULT WINAPI FolderItemsImpl_get_Count(FolderItems3 *iface, LONG *count)
> >  {
> > - FIXME("(%p,%p)\n", iface, count);
> > - FolderItemsImpl *This = impl_from_FolderItems(iface);
> > - return E_NOTIMPL;
> > - TRACE("(%p,%p)\n", iface, count);
> > -
> > - *count = PathIsDirectoryW(V_BSTR(&This->dir)) ? This->item_count : 0;
> > - return S_OK;
> >
> > }
>
> This looks too complicated. Why can't we always return item_count
> without additional checks?

FolderItems_get_Count returns 0 if the directory was deleted. There is
a test for this.

-Alex

[PATCH 3/3] shell32: Implement FolderItems_get_Count.

[Nikolay Sivov]

On 11.09.2017 6:39, Alex Henrie wrote:
> 2017-09-08 2:30 GMT-06:00 Nikolay Sivov :
> > On 08.09.2017 8:29, Alex Henrie wrote:
> > > static HRESULT WINAPI FolderItemsImpl_get_Count(FolderItems3 *iface, LONG *count)
> > >  {
> > > - FIXME("(%p,%p)\n", iface, count);
> > > - FolderItemsImpl *This = impl_from_FolderItems(iface);
> > > - return E_NOTIMPL;
> > > - TRACE("(%p,%p)\n", iface, count);
> > > -
> > > - *count = PathIsDirectoryW(V_BSTR(&This->dir)) ? This->item_count : 0;
> > > - return S_OK;
> > >
> > > }
> >
> > This looks too complicated. Why can't we always return item_count
> > without additional checks?
>
> FolderItems_get_Count returns 0 if the directory was deleted. There is
> a test for this.

I see, but if directory is recreated, we'll be returning count that's no
longer relevant. It will still match snapshot size I suppose, does it
work like that? Currently test is removing directory, testing if count
turns to 0 and if Item() method stops working for integer indexing. I'm
only suggesting this as a future improvement, because it's not obvious
how any of that works, until you try.
> -Alex

[PATCH 3/3] shell32: Implement FolderItems_get_Count.

[Alex Henrie]

2017-09-11 0:15 GMT-06:00 Nikolay Sivov :
> On 11.09.2017 6:39, Alex Henrie wrote:
> > 2017-09-08 2:30 GMT-06:00 Nikolay Sivov :
> > > On 08.09.2017 8:29, Alex Henrie wrote:
> > > > static HRESULT WINAPI FolderItemsImpl_get_Count(FolderItems3 *iface, LONG *count)
> > > >  {
> > > > - FIXME("(%p,%p)\n", iface, count);
> > > > - FolderItemsImpl *This = impl_from_FolderItems(iface);
> > > > - return E_NOTIMPL;
> > > > - TRACE("(%p,%p)\n", iface, count);
> > > > -
> > > > - *count = PathIsDirectoryW(V_BSTR(&This->dir)) ? This->item_count : 0;
> > > > - return S_OK;
> > > >
> > > > }
> > >
> > > This looks too complicated. Why can't we always return item_count
> > > without additional checks?
> >
> > FolderItems_get_Count returns 0 if the directory was deleted. There is
> > a test for this.
>
> I see, but if directory is recreated, we'll be returning count that's no
> longer relevant. It will still match snapshot size I suppose, does it
> work like that? Currently test is removing directory, testing if count
> turns to 0 and if Item() method stops working for integer indexing. I'm
> only suggesting this as a future improvement, because it's not obvious
> how any of that works, until you try.

I'm pretty sure that the current behavior is correct, because the
count goes out of sync with reality every time a file is added to or
deleted from the directory. But like you said, testing what happens
after the directory itself is deleted and recreated isn't really
necessary for an initial implementation.

-Alex