David Quintana

Given that VS2010 is the newest one that has a chance to work in ReactOS, that may take a while.

Given that VS2010 is the newest one that has a chance to work in ReactOS, that may take a while.

Yep seems like I was thinking of something else. Doing a quick search through our CMakeLists.txt files, it seems like we do this in quite a few places, so it's OK.

Yep seems like I was thinking of something else. Doing a quick search through our CMakeLists.txt files, it seems like we do this in quite a few places, so it's OK.

don't we have some "add_include_files" for this?

don't we have some "add_include_files" for this?

In that case it would be best to have some prefix, like LicenseType_OpenSource and so on, rather than "polluting" the global namespace with names such as "Min" or "None".

In that case it would be best to have some prefix, like LicenseType_OpenSource and so on, rather than "polluting" the global namespace with names such as "Min" or "None".

This looks a lot like a C# enum. It doesn't take into account that C++ enums are not namespaced, which means it puts "None", "OpenSource" etc into the global namespace. There is "enum class" in C++...

This looks a lot like a C# enum. It doesn't take into account that C++ enums are not namespaced, which means it puts "None", "OpenSource" etc into the global namespace. There is "enum class" in C++11 which is namespaced, but I'm not sure if VS2010 supports that. Regardless of style choice, this would have to be fixed one way or another.

Nothing wrong with this, just wanted to note that we have a CToolbar class in rosctls.h, which could be used to reduce the amount of code and improve readability.

Nothing wrong with this, just wanted to note that we have a CToolbar class in rosctls.h, which could be used to reduce the amount of code and improve readability.

ACK. Will change before next iteration ;P

ACK. Will change before next iteration ;P

I tried, but since the subclass also has CComCoClass, there's multiple definitions of the factory stuff, which results in ambiguous references in the OBJECT_MAP defined at shell32_main.cpp If there...

I tried, but since the subclass also has CComCoClass, there's multiple definitions of the factory stuff, which results in ambiguous references in the OBJECT_MAP defined at shell32_main.cpp
If there's a better way to do this, I'd be glad to know.

Since I copied CBaseBar from the browseui version, should I also fix these comments there?

Since I copied CBaseBar from the browseui version, should I also fix these comments there?

Oh crap I included changes from the icon cache stuff.

Oh crap I included changes from the icon cache stuff.

The code is highly WIP, still. It's here so that you can yell to me if I did something horribly wrong ;P

The code is highly WIP, still. It's here so that you can yell to me if I did something horribly wrong ;P

  • More
  • CR-43
  • started review
shell-classes-it6.patch
shell-classes-it6.patch
  • More
  • CR-39
  • commented on review
The changes in this file are unrelated. This was the first thing I looked at from the shell, and MSDN mentioned these pointers were optional, except pdwFlags.

The changes in this file are unrelated. This was the first thing I looked at from the shell, and MSDN mentioned these pointers were optional, except pdwFlags.

  • More
  • CR-39
  • commented on review
For debugging only ;P

For debugging only ;P

You have inconsistent location of the opening braces. I think the convention in ReactOS is to place them at the next line? At least it is for the rest of the shell.

You have inconsistent location of the opening braces. I think the convention in ReactOS is to place them at the next line? At least it is for the rest of the shell.

Yes... it is.

Yes... it is.

  • More
  • CR-39
  • commented on review
Regarding this function: I chose to use an Event handle parameter as an indicator of completion, but I don't know what the preferred method of asynchronous code would be. The alternative I thought...

Regarding this function: I chose to use an Event handle parameter as an indicator of completion, but I don't know what the preferred method of asynchronous code would be.

The alternative I thought was to use QueueUserAPC on the calling thread, with the data parameter set to the IcAsyncState pointer.

I was going to use that, but I remembered some talk where a dude explained emergent structures in C++ code, and one of the things he said was that sometimes giving the compiler pointers/references ...

I was going to use that, but I remembered some talk where a dude explained emergent structures in C++ code, and one of the things he said was that sometimes giving the compiler pointers/references instead of plain values prevented the compiler from doing certain optimizations, so given the doubt, I chose the version that required the least typing. ;P

No it is not compatible. If it's really wanted, I could try to figure it out, though.

No it is not compatible. If it's really wanted, I could try to figure it out, though.

I wrote this when I was still using an std::map, I can make it a static function in the .cpp instead. I think it's still POD, since that should simply generate a special function when it compiles, ...

I wrote this when I was still using an std::map, I can make it a static function in the .cpp instead.
I think it's still POD, since that should simply generate a special function when it compiles, but I will make it cleaner by creating a KeyCompare() function instead.

I wrote the rest of the code using lowercase parameters, the above function prototypes are copypasted from the msdn. The rest of shell32 is not consistent, if there is a "new code" style to be used...

I wrote the rest of the code using lowercase parameters, the above function prototypes are copypasted from the msdn.
The rest of shell32 is not consistent, if there is a "new code" style to be used I will change things accordingly.

ugh... copypasta from msdn ;P

ugh... copypasta from msdn ;P