CVS HubFrame Cleanup

Problems compiling? Don't understand the source code? Don't know how to code your feature? Post here.

Moderator: Moderators

Locked
Pothead
Posts: 223
Joined: 2005-01-15 06:55

CVS HubFrame Cleanup

Post by Pothead » 2005-11-14 06:39

I recently send the following patch to arnetheduck, to fix a bug with Share Size column not sorting in the CVS (i think it was Guitarm who noticed this :))

www.freetohost.co.uk/mike/patches/HubFrameShareSize.txt
arnetheduck wrote:this means that bytes can be removed from UserInfo as well (which I did) and that op and hidden should be read from identity as well (and removed)
I've had a go with this, but i would like to ask some advice.

hidden can be removed quite simply, but one of 3 changes (which i can think of) need to be done, and i was wondering which would be the "best way" to do it.
Identity in UserInfo is in the private section, so cannot be accessed by HubFrame.
1. Change it to Protected (i think would be best)
2. Change it to Public
3. Declare HubFrame a friend class of Userinfo.

Also removing Op seems a bit tricker, as i cannot see how to do it without breaking the following function

Code: Select all

bool HubFrame::UserInfo::update(const Identity& identity, int sortCol) {
	bool needsSort = (op != identity.isOp());
Any advice would be welcome. :)

GargoyleMT
DC++ Contributor
Posts: 3212
Joined: 2003-01-07 21:46
Location: .pa.us

Re: CVS HubFrame Cleanup

Post by GargoyleMT » 2005-11-14 12:05

Pothead wrote:Identity in UserInfo is in the private section, so cannot be accessed by HubFrame.
1. Change it to Protected (i think would be best)
2. Change it to Public
3. Declare HubFrame a friend class of Userinfo.
The fourth option would be to build a function to give access to it, as is done in (some) other portions of DC++. (I'm not sure which is best.)
Pothead wrote:Also removing Op seems a bit trickier, as i cannot see how to do it without breaking the following function
Well, the code there seems to serve only one purpose: to detect when the user has become an operator (or ceases to be one). Unless it is used elsewhere, I'd rename the variable to something more intuitive, and leave it as is.

Pothead
Posts: 223
Joined: 2005-01-15 06:55

Re: CVS HubFrame Cleanup

Post by Pothead » 2005-11-15 07:09

GargoyleMT wrote:The fourth option would be to build a function to give access to it, as is done in (some) other portions of DC++. (I'm not sure which is best.)
Yes, good point i forgot about this. I seen in one or two places as well. Maybe best if i supply a few patches, doing different ways ?
GargoyleMT wrote:Well, the code there seems to serve only one purpose: to detect when the user has become an operator (or ceases to be one). Unless it is used elsewhere, I'd rename the variable to something more intuitive, and leave it as is.
The elsewhere places it's used (if any, cannot remember), can all be changed to use Identity.isOp(), except this one, which i couldn't see how to changed without breaking it. :)

GargoyleMT
DC++ Contributor
Posts: 3212
Joined: 2003-01-07 21:46
Location: .pa.us

Re: CVS HubFrame Cleanup

Post by GargoyleMT » 2005-11-15 11:26

Pothead wrote:The elsewhere places it's used (if any, cannot remember), can all be changed to use Identity.isOp(), except this one, which i couldn't see how to changed without breaking it. :)
The goal is to preserve the functionality. Well, first check to make sure there is functionality - remove it and make sure DC++ doesn't sort properly without it on a big hub with operators and users joining and leaving. If the code is functional, then you either need to keep it or devise a replacement. Since the code relies on duplication of operator status in two objects to detect when that flag changes, simply using the identity class isn't possible.

FarCry
Programmer
Posts: 34
Joined: 2003-05-01 10:49

Re: CVS HubFrame Cleanup

Post by FarCry » 2005-11-19 05:44

GargoyleMT wrote:Since the code relies on duplication of operator status in two objects to detect when that flag changes, simply using the identity class isn't possible.
The "identity" local variable is not the same as the "identity" UserInfo member in that function, but a reference to the updated identity. A change could be detected by comparing getIdentity().isOP() against identity.isOp().

Pothead
Posts: 223
Joined: 2005-01-15 06:55

Post by Pothead » 2005-12-02 12:52

Thanks GargoyleMT and Farcry for the advice. I've created this patch, which seems to work fine.

Code: Select all

diff -urdN dcpp orig/windows/HubFrame.cpp dcpp new/windows/HubFrame.cpp
--- dcpp orig/windows/HubFrame.cpp	Tue Nov 29 15:08:21 2005
+++ dcpp new/windows/HubFrame.cpp	Fri Dec  2 17:40:39 2005
@@ -351,12 +351,12 @@
 	if(i == userMap.end()) {
 		UserInfo* ui = new UserInfo(u);
 		userMap.insert(make_pair(u.user, ui));
-		if(!ui->getHidden() && showUsers)
+		if(!ui->getIdentity().isHidden() && showUsers)
 			ctrlUsers.insertItem(ui, getImage(u.identity));
 		return true;
 	} else {
 		UserInfo* ui = i->second;
-		if(!ui->getHidden() && u.identity.isHidden() && showUsers) {
+		if(!ui->getIdentity().isHidden() && u.identity.isHidden() && showUsers) {
 			ctrlUsers.deleteItem(ui);
 		}
 		
@@ -377,7 +377,7 @@
 	dcassert(i != userMap.end());
 
 	UserInfo* ui = i->second;
-	if(!ui->getHidden() && showUsers)
+	if(!ui->getIdentity().isHidden() && showUsers)
 		ctrlUsers.deleteItem(ui);
 
 	userMap.erase(i);
@@ -385,7 +385,7 @@
 }
 
 bool HubFrame::UserInfo::update(const Identity& identity, int sortCol) {
-	bool needsSort = (op != identity.isOp());
+	bool needsSort = (getIdentity().isOp() != identity.isOp());
 	tstring old;
 	if(sortCol != -1)
 		old = columns[sortCol];
@@ -397,9 +397,6 @@
 	/// @todo columns[COLUMN_CONNECTION] = Text::toT(i->getConnection());
 	columns[COLUMN_EMAIL] = Text::toT(identity.getEmail());
 
-	op = identity.isOp();
-	hidden = identity.isHidden();
-
 	if(sortCol != -1) {
 		needsSort = needsSort || (old != columns[sortCol]);
 	}
@@ -996,7 +993,7 @@
 		
 		for(UserMapIter i = userMap.begin(); i != userMap.end(); ++i) {
 			UserInfo* ui = i->second;
-			if(!ui->getHidden())
+			if(!ui->getIdentity().isHidden())
 				ctrlUsers.insertItem(ui, getImage(ui->getIdentity()));
 		}
 
diff -urdN dcpp orig/windows/HubFrame.h dcpp new/windows/HubFrame.h
--- dcpp orig/windows/HubFrame.h	Tue Nov 29 15:08:22 2005
+++ dcpp new/windows/HubFrame.h	Fri Dec  2 17:01:30 2005
@@ -193,7 +193,7 @@
 	friend struct CompareItems;
 	class UserInfo : public UserInfoBase, public FastAlloc<UserInfo> {
 	public:
-		UserInfo(const UpdateInfo& u) : UserInfoBase(u.user), op(false), hidden(false) { 
+		UserInfo(const UpdateInfo& u) : UserInfoBase(u.user) { 
 			update(u.identity, -1); 
 		};
 
@@ -203,9 +203,9 @@
 
 		static int compareItems(const UserInfo* a, const UserInfo* b, int col) {
 			if(col == COLUMN_NICK) {
-				if(a->getOp() && !b->getOp()) {
+				if(a->getIdentity().isOp() && !b->getIdentity().isOp()) {
 					return -1;
-				} else if(!a->getOp() && b->getOp()) {
+				} else if(!a->getIdentity().isOp() && b->getIdentity().isOp()) {
 					return 1;
 				}
 			}
@@ -219,8 +219,6 @@
 
 		tstring columns[COLUMN_LAST];
 		GETSET(Identity, identity, Identity);
-		GETSET(bool, op, Op);
-		GETSET(bool, hidden, Hidden);
 	};
 
 	class PMInfo {
If this looks right i'll enter into Bugzilla. :)

I am also assuming that

Code: Select all

   op = identity.isOp();
   hidden = identity.isHidden(); 
is no longer needed, as this should be done a few lines lower, with existing

Code: Select all

	setIdentity(identity);

GargoyleMT
DC++ Contributor
Posts: 3212
Joined: 2003-01-07 21:46
Location: .pa.us

Post by GargoyleMT » 2005-12-02 13:01

Visually, the patch looks fine, but I'm not in a frame of mind where I can give a particularly critical review.

Locked