"Share Hidden" - A code review please?

[TheParanoidOne]

Hello all!

So, I'm now able to compile the current version of DC++ (cheers to everyone who responded to my plea for help ) and decided to dive in, seeing as I am currently "between jobs".

I wanted to go for something obscure and out of the way that no-one would notice, so I picked the "Share Hidden" checkbox in the Sharing page. Obscure enough?
It started off as trying to fix a bug from the tracker but then I realised there was a deficiency on this page. If you add or remove a directory from your share, the share size is updated immediately in the GUI. The Share Hidden button does not do this though. You need to leave settings, manually refresh the share and then go back to settings to see the changes.

So I decided to try and implement that. And all hell broke loose. It was a spiralling nightmare, but I think I'm the better for it, seeing as this is the first time I've ever delved into a C++ project. It may only be a few lines of code, but it has helped me greatly with understanding of some of the inner workings of DC++ and also the use of C++.

"The point?" I hear you ask. Well basically, I just want someone to go over what I have done and see if there are any glaring errors in the way that I have done it. Plus any other constructive criticism.

So here is the code:

Code: Select all

diff -c DCPlusPlus-0.304-src/windows/UploadPage.cpp DC++/windows/UploadPage.cpp
*** DCPlusPlus-0.304-src/windows/UploadPage.cpp Mon Oct 27 18:10:54 2003
--- DC++/windows/UploadPage.cpp Fri Nov 21 22:50:39 2003
***************
*** 138,143 ****
--- 138,165 ----
        return 0;
  }
  
+ LRESULT UploadPage::onClickedShareHidden(WORD /*wNotifyCode*/, WORD /*wID*/, HWND /*hWndCtl*/, BOOL& /*bHandled*/)
+ {
+       // Save this pages info so that ShareManager knows to include/disclude hidden files
+       PropPage::write((HWND)*this, items);
+       // Refresh the share. This is a blocking refresh. Might cause problems?
+       ShareManager::getInstance()->setDirty();
+       ShareManager::getInstance()->refresh(true, false, true);
+ 
+       // Clear the GUI list, for insertion of updated shares
+       ctrlDirectories.DeleteAllItems();
+       StringList directories = ShareManager::getInstance()->getDirectories();
+       for(StringIter j = directories.begin(); j != directories.end(); j++)
+       {
+               int i = ctrlDirectories.insert(ctrlDirectories.GetItemCount(), *j);
+               ctrlDirectories.SetItemText(i, 1, Util::formatBytes(ShareManager::getInstance()->getShareSize(*j)).c_str());
+       }
+ 
+       // Display the new total share size
+       ctrlTotal.SetWindowText(Util::formatBytes(ShareManager::getInstance()->getShareSize()).c_str());
+       return 0;
+ }
+ 
  /**
   * @file
   * $Id: UploadPage.cpp,v 1.12 2003/10/27 17:10:53 arnetheduck Exp $

Code: Select all

diff -c DCPlusPlus-0.304-src/windows/UploadPage.h DC++/windows/UploadPage.h
*** DCPlusPlus-0.304-src/windows/UploadPage.h   Mon Oct 20 23:04:56 2003
--- DC++/windows/UploadPage.h   Thu Nov 20 23:28:21 2003
***************
*** 42,53 ****
--- 42,55 ----
                NOTIFY_HANDLER(IDC_DIRECTORIES, LVN_ITEMCHANGED, onItemchangedDirectories)
                COMMAND_ID_HANDLER(IDC_ADD, onClickedAdd)
                COMMAND_ID_HANDLER(IDC_REMOVE, onClickedRemove)
+               COMMAND_ID_HANDLER(IDC_SHAREHIDDEN, onClickedShareHidden)
        END_MSG_MAP()
  
        LRESULT onInitDialog(UINT, WPARAM, LPARAM, BOOL&);
        LRESULT onItemchangedDirectories(int idCtrl, LPNMHDR pnmh, BOOL& bHandled);
        LRESULT onClickedAdd(WORD wNotifyCode, WORD wID, HWND hWndCtl, BOOL& bHandled);
        LRESULT onClickedRemove(WORD wNotifyCode, WORD wID, HWND hWndCtl, BOOL& bHandled);
+       LRESULT onClickedShareHidden(WORD wNotifyCode, WORD wID, HWND hWndCtl, BOOL& bHandled);
  
        // Common PropPage interface
        PROPSHEETPAGE *getPSP() { return (PROPSHEETPAGE *)*this; }

Cheers!

[GargoyleMT]

I read your post and am thinking it over.

My first reaction was that the write() seems a bit premature, but Adding/Removing directories does happen immediately also. The Cancel button cannot back out changes you making in the Sharing dialog anyway.

I guess also that the full refresh might be the best way to do it... if people don't toggle that a lot, it would be pointless to try to optimize that case... (ie. perhaps scan all the files anyway, and set an internal bool for including in the list anyway.


As far as changes, I'd probably change the write() to a set of only the share-hidden member, that way cancel can still operate on upload slots.

I'm probably missing several things in this post, I'll see if anything comes to mind later. ^_^

[TheParanoidOne]

My first reaction was that the write() seems a bit premature

Totally agreed. It most certainly *is* overkill. What I wanted to do was check the value of the checkbox and then save that setting. But I couldn't figure out how to get that info. I would think that is is done with ::IsDlgButtonChecked, seeing as that is how write() does it, but I just couldn't get it to work, so I took the sledgehammer approach and just saved everything.

I guess also that the full refresh might be the best way to do it... if people don't toggle that a lot, it would be pointless to try to optimize that case... (ie. perhaps scan all the files anyway, and set an internal bool for including in the list anyway.

A full refresh seems to be the only way to go as far as I can see. Hopefully people won't be clicking this button constantly! My only worry here is the fact that the refresh is blocking which freezes the GUI. In my tests this was never a problem, but it may take longer for other people.

As far as changes, I'd probably change the write() to a set of only the share-hidden member, that way cancel can still operate on upload slots.

Agreed, once I figure out how.

I'm probably missing several things in this post, I'll see if anything comes to mind later. ^_^

I look forward to it.

Something that I wanted to do with the directory list once the refresh was done, was to update the size of each directory by iterating through each entry in the list, and settings it's display value. I hit a brick wall here though, as I cannot get a specific item. As far as I can see, I get get the number of items in the list as well as get the first item, but that's it. So in the end I just cleared the list and added them all again. I guess it makes no difference though, seeing as with both approaches I would be working on every entry.

A lot of the brick walls I am hitting seem to be caused by lack of any API documenatation whatsoever. The items in the code completion dropdown boxes do not cover all the possible completions that there are and so, without adequate API documentation, there is no way to know what methods are available for a particular object. Very frustrating.

[GargoyleMT]

Something that I wanted to do with the directory list once the refresh was done, was to update the size of each directory by iterating through each entry in the list, and settings it's display value. I hit a brick wall here though, as I cannot get a specific item. As far as I can see, I get get the number of items in the list as well as get the first item, but that's it. So in the end I just cleared the list and added them all again. I guess it makes no difference though, seeing as with both approaches I would be working on every entry.

Ok, this is a pretty pathetic response (I'm trying to kill spare time at work), but you could always try listening to the LVN_GETDISPINFO message, which is how recent changes to the TransferView and HubFrame have updated specific columns in listviews. Take a look at the message map in the .h files.