Author Topic: clist_modern. Incorrect account menus, if some accounts hidden  (Read 4879 times)

0 Members and 1 Guest are viewing this topic.

Offline l.incTopic starter

  • Newbie
  • *
  • Posts: 15
  • Karma: 2
Hello, new community! Exactly two years ago I posted a bug report including a patch. I had a great pleasure to find out today, that it had been incorporated into the miranda-ng together with my feature request from the same post. In fact I was even more pleased to discover the very existence of the miranda-ng project today.

Switching to miranda-ng and configuring it brought me across another bug in some neighboring code. If an account is hidden (using per account options in Options->Contact list->Status Bar) and the account is ordered in between some visible accounts (using Options->Contact list->Accounts), then wrong menus are shown for the following visible accounts in the status bar.

I attach a diff fixing the bug (based upon the git commit 6a1286e47d52fb2146c2a283527dc4d60743ba87), but I read/edited the source in the notepad, as I currently don't have Visual Studio installed (new machine). Thus not tested, but as long as the modification is quite little and I always write bugfree code... :-)

Kind regards
 

Offline Wishmaster

  • Developer
  • Global Moderator
  • *****
  • Posts: 650
  • Country: de
  • Karma: 31
  • Anti-Hero
  • Version Info
No, patch is wrong. It fixes your bug but it creates a new one: If account display order is changed (Options->Contact List->Accounts), the menus are wrong.
« Last Edit: 29 04 2014, 16:34:08 by Wishmaster »
 

Offline l.incTopic starter

  • Newbie
  • *
  • Posts: 15
  • Karma: 2
Wishmaster
I actually was sure I read the related code correctly. I therefore spent a day restoring the development infrastructure, I got used to, on my new machine in order to test my patch. And the thing is, I don't observe incorrect menus no matter what account ordering I use. I however applied the modification to the 0.94.9.

Did you really test the patch, or did you just look into the code? If you just feel concerned about that loop removed, then it was an absolute nonsense code.
 

Offline Wishmaster

  • Developer
  • Global Moderator
  • *****
  • Posts: 650
  • Country: de
  • Karma: 31
  • Anti-Hero
  • Version Info

Did you really test the patch, or did you just look into the code? If you just feel concerned about that loop removed, then it was an absolute nonsense code.
I did test it with a development build and it didnt work. Try hiding an account and to changr the display order.
 

Offline l.incTopic starter

  • Newbie
  • *
  • Posts: 15
  • Karma: 2
Wishmaster
Already did. Five accounts, two accounts hidden, nearly all possible orders checked. Menus are always correct. Would you want me to attach the binary for 0.94.9?
 

Offline Robyer

  • Hero Member
  • *****
  • Posts: 1082
  • Country: cz
  • Karma: 60
    • Robyer.cz website
  • Version Info
l.inc, make sure it works in development version. It doesn't make sense to write anything for stable branch. ;-)
I was developing mainly Facebook, Omegle, Steam, Dummy and MobileState plugins. Now I'm retired. Goodbye, everyone. ~ You can still find me on Facebook.
 

Offline Wishmaster

  • Developer
  • Global Moderator
  • *****
  • Posts: 650
  • Country: de
  • Karma: 31
  • Anti-Hero
  • Version Info
Wishmaster
Already did. Five accounts, two accounts hidden, nearly all possible orders checked. Menus are always correct. Would you want me to attach the binary for 0.94.9?
Well, please test with trunk version.

I applied patch again and tested, see screenshot:

Note that it doesn't happen with official version. Patch is rejected.
 

Offline l.incTopic starter

  • Newbie
  • *
  • Posts: 15
  • Karma: 2
Wishmaster
Quote (selected)
see screenshot
Oh, you mean this way of hiding. I've been using the per account options on the status bar tab. Yes... You're right. Fixing one bug triggers another bug, because the code is quite sloppy and needs revision anyway. E.g., there's a memory leak in case the accounts are hidden on the status bar only (using Options->Contact list->Status Bar), as the ProtoItemData is in this case allocated, but not added to the ProtosData list.

The main problem is here however, that multiple code pieces need to have same logic relying on pfnGetProtocolVisibility: the one, that builds the status menu (from the miranda core), and the one, that takes submenus from the status menu (from a contact list plugin). This is a generally bad approach, because it requires code replication to some extent.

I'm gonna improve the code a bit, but someone really needs to improve the overall architecture and clearly define interfaces before proceeding on adding more and more features, while incorporating exponentially more bugs. E.g., how should I know, that I need to call pcli->pfnGetProtocolVisibility, if there's a directly available flag bIsVisible in the PROTOACCOUNT structure? I can see fnGetProtocolVisibility does more, than just checking the flag, but the fields of the PROTOACCOUNT must not be directly available at all. This way one could clearly see, that some other way is meant for the visibility check. Performance would be also improved this way, because one does not need an additional lookup by the account name, when already having a pointer to the structure (it would better be a class not structure).
« Last Edit: 30 04 2014, 20:57:16 by l.inc »
 

Offline Wishmaster

  • Developer
  • Global Moderator
  • *****
  • Posts: 650
  • Country: de
  • Karma: 31
  • Anti-Hero
  • Version Info
I'm gonna improve the code a bit, but someone really needs to improve the overall architecture and clearly define interfaces before proceeding on adding more and more features, while incorporating exponentially more bugs. E.g., how should I know, that I need to call pcli->pfnGetProtocolVisibility, if there's a directly available flag bIsVisible in the PROTOACCOUNT structure? I can see fnGetProtocolVisibility does more, than just checking the flag, but the fields of the PROTOACCOUNT must not be directly available at all. This way one could clearly see, that some other way is meant for the visibility check. Performance would be also improved this way, because one does not need an additional lookup by the account name, when already having a pointer to the structure (it would better be a class not structure).
Nope, PROTOACCOUNT members must be directly available, there are a lot of other plugins using it. It is a very central part of accounts API. As you can see pfnGetProtocolVisibility does something else then checking that flag, and this is only to be used inside the clist plugins.
 

Offline l.incTopic starter

  • Newbie
  • *
  • Posts: 15
  • Karma: 2
Wishmaster
I'm sorry, I don't think I'll be able to keep up with the discussion, because I don't have a good general picture of miranda's architecture. But "a lot of other plugins using it" sounds like a result of an initially bad design decision, rather than a reason to maintain it. Anyway I failed to find those lots of plugins using for example the bIsVisible flag. I found only two: W7UI declared deprecated and SimpleStatusMsg which seems to just reimplement the functionality of fnGetProtocolVisibility in an unfortunate way. I.e. if we look at the following piece of code:
Code: [Select]
for (int i = 0; i < accounts->count; ++i)
{
if (!IsAccountEnabled(accounts->pa[i]))
continue;

if (!CallProtoService(accounts->pa[i]->szModuleName, PS_GETCAPS, PFLAGNUM_3, 0))
continue;

if (!(CallProtoService(accounts->pa[i]->szModuleName, PS_GETCAPS, PFLAGNUM_1, 0) & PF1_MODEMSGSEND))
continue;

if (!accounts->pa[i]->bIsVisible)
continue;

if (hProtoStatusMenuItem[i] == (HANDLE)lParam)
{
box_data->m_szProto = accounts->pa[i]->szModuleName;
box_data->m_iStatusModes = CallProtoService(accounts->pa[i]->szModuleName, PS_GETCAPS, PFLAGNUM_2, 0)&~CallProtoService(accounts->pa[i]->szModuleName, PS_GETCAPS, PFLAGNUM_5, 0);
box_data->m_iStatusMsgModes = CallProtoService(accounts->pa[i]->szModuleName, PS_GETCAPS, PFLAGNUM_3, 0);

idvstatusmsg = TRUE;
break;
}
}
then we see, that it does five (!) lookups by name for the information directly available from the PROTOACCOUNT structure. Using a utility function similar to fnGetProtocolVisibility could reduce the number of lookups to zero, if it was taking a PROTOACCOUNT pointer only. Moreover there would be no need to access the fields of PROTOACCOUNT directly.
Quote (selected)
As you can see pfnGetProtocolVisibility does something else then checking that flag
It doesn't do something else. It just does more. Btw. it's functionality is limited to evaluating the fields of the structure. A careless programmer could decide to reimplement the functionality just because an instance of the PROTOACCOUNT structure is already available to him (why doing a lookup by name, right?). And that would be a result of making the fields of the structure public.


For the reason specified in the first sentence of this post I refrained from some large code improvements and only fixed the two bugs: memory leak and wrong contact menus. The patch is attached. I was surprised to find out, that there's no way to retrieve a native menu handle for an account. There is cli.pfnGetProtocolMenu, but for unclear reasons the structure defined by the returned handle (HGENMENU) does not contain the corresponding native handle. There is an ugly (fortunately nowhere used) function FindMenuHanleByGlobalID, that does not even match it's own prototype from genmenu.h, which performs a reverse lookup. But aside from it's ugliness it's also not publicly accessible, which is good, btw.

P.S. I didn't manage to compile an installer for the latest development version and didn't want to tamper with my stable installation by converting it into some development mix. Thus I could not test the patch, but it should work correctly now.
 

Offline ghazan

  • Miranda NG founder
  • Administrator
  • *****
  • Posts: 719
  • Country: ru
  • Karma: 46
  • Jabber ID
l.inc, I would prefer to remove that option completely. account visibility must be controlled as all decent people do, using Options - Contact list - Accounts

that strange option in clist_modern only duplcates the existing functionality, and therefore must be removed
 

Offline l.incTopic starter

  • Newbie
  • *
  • Posts: 15
  • Karma: 2
ghazan
Quote (selected)
that strange option in clist_modern only duplcates the existing functionality
It does not. The existing functionality is completely hiding the account, meaning from both the status bar and the account status menu. The clist_modern option allows to hide accounts from status bar only leaving those in the status menu, which I personally find very useful.
 

Offline Wishmaster

  • Developer
  • Global Moderator
  • *****
  • Posts: 650
  • Country: de
  • Karma: 31
  • Anti-Hero
  • Version Info
then we see, that it does five (!) lookups by name for the information directly available from the PROTOACCOUNT structure. Using a utility function similar to fnGetProtocolVisibility could reduce the number of lookups to zero, if it was taking a PROTOACCOUNT pointer only. Moreover there would be no need to access the fields of PROTOACCOUNT directly.It doesn't do something else. It just does more. Btw. it's functionality is limited to evaluating the fields of the structure. A careless programmer could decide to reimplement the functionality just because an instance of the PROTOACCOUNT structure is already available to him (why doing a lookup by name, right?). And that would be a result of making the fields of the structure public.
It is doing the CallProtoService to call the protocol services, take a look at m_protosvc.h
Quote (selected)
#define PF1_MODEMSGSEND   0x00000040       //supports broadcasting away messages
#define PF1_MODEMSGRECV   0x00000080       //supports reading others' away messages
#define PF1_MODEMSG       (PF1_MODEMSGSEND|PF1_MODEMSGRECV)

//the status modes that the protocol supports
//away-style messages for. Uses the PF2_ flags.
// PFLAGNUM_3 is implemented by protocol services that support away messages
// there may be no support and 0 will be returned, if there is
// support it will consist of a set of PF2_* bits
#define PFLAGNUM_3   3
Thats something different (yes, something more means something different) then just bIsVisible


About the patch, I comitted it. :)
« Last Edit: 01 05 2014, 18:13:05 by Wishmaster »
 

Offline l.incTopic starter

  • Newbie
  • *
  • Posts: 15
  • Karma: 2
Wishmaster
Quote (selected)
It is doing the CallProtoService to call the protocol services
Yes. And CallProtoService results in the GetCaps call (see CallProtoService->CallProtoServiceInt in "src\modules\protocols\protocols.cpp"), same as fnGetProtocolVisibility does, but opposed to the direct call by pointer specified in the PROTOACCOUNT requires additional lookups by name for the structure.

Regarding the commit, thank you.  :)
« Last Edit: 01 05 2014, 19:09:54 by l.inc »
 

Offline White-Tiger

  • Developer
  • *****
  • Posts: 182
  • Country: 00
  • Karma: 10
  • SendSS maintainer
[...]
Regarding the commit, thank you.  :)
did you check that commit? :P Cuz he only commited a lil' part of it... and that one even "redone"^^