Issue 29332455: Issue #1596, #1720 - Make status bar menu consistent with plugin abilities (Closed)
DescriptionIssue #1596, #1720 - Make status bar menu consistent with plugin abilities
Add 'CPluginTab::IsPossibleToDisableOnSite()'. This function returns true if
the tab document has protocol "http:" or "https:", the only protocols that ABP
is able to intercept. Issue #1596 concerns protocol "about:"; issue #1720
concerns protocol "file:".
Add condition to call 'IsPossibleToDisableOnSite()' when displaying pop-up
menu, disabling the menu item to disable if unable to disable.
Patch Set 1 #
Total comments: 6
Patch Set 2 : address comments #
Total comments: 4
MessagesTotal messages: 9
The function 'CanDisableOnSite()' could have been inlined, it's true. I placed it in 'CPluginTab' in order to improve separation of responsibilities in the code base and also, more specifically, in anticipation of #3394.
Actually, firstly we should discuss in the issue whether we want the menu item to be disabled or removed from the menu in such cases. https://codereview.adblockplus.org/29332455/diff/29332456/src/plugin/PluginTa... File src/plugin/PluginTabBase.h (right): https://codereview.adblockplus.org/29332455/diff/29332456/src/plugin/PluginTa... src/plugin/PluginTabBase.h:79: * Is it possible to disable the current content on a per-site basis? This comment is unclear because it's not clear what the current content it is told here about and it's not question whether it's possible or not on a per-site basis. Actually the method should indicate whether it makes sense and possible to either enable or disable ABP on the current web site. https://codereview.adblockplus.org/29332455/diff/29332456/src/plugin/PluginTa... src/plugin/PluginTabBase.h:81: * ABP only intercepts protocols "http:" and "https:", I would rather move it into the method body because it explains the magic strings used there. https://codereview.adblockplus.org/29332455/diff/29332456/src/plugin/PluginTa... src/plugin/PluginTabBase.h:82: * so it makes no sense to offer the user an option to block content from other protocols. I bit unusual formatting with two spaces gap.
> Actually, firstly we should discuss in the issue whether we want the > menu item to be disabled or removed from the menu in such cases. Regardless of what choice we make, there's a possible follow-up change set to this one. (A) If we disable, we'd ideally have a tooltip explaining this menu item, with text for each of its three display states. In addition to the code, that would also require translation strings. (B) If we remove, we need code to do it. So I'd rather not have a big discussion about the UI design right now. The present change set fixes two issues. If we want a different UI after this change set set, we can make a different UI. I don't want to hold a defect fix hostage to a design discussion about a small UI issue. We don't have any kind of rule that this is the last modification to the code. Personally, I think in this case that disabling is better than removing. As a rule, minor variations in content should see a menu item disabled. Major variations should see a different menu (such as removing an item). This counts as a minor variation, so I wrote it to disable the menu item. https://codereview.adblockplus.org/29332455/diff/29332456/src/plugin/PluginTa... File src/plugin/PluginTabBase.h (right): https://codereview.adblockplus.org/29332455/diff/29332456/src/plugin/PluginTa... src/plugin/PluginTabBase.h:79: * Is it possible to disable the current content on a per-site basis? On 2015/12/08 20:20:19, sergei wrote: > This comment is unclear because it's not clear what the current content it is > told here about and it's not question whether it's possible or not on a per-site > basis. Tabs have current content displayed in the browser tab window. What's confusing about that? > Actually the method should indicate whether it makes sense and possible to > either enable or disable ABP on the current web site. Well, that's what it says in the function name. I've renamed the function to avoid the use of the English modal verb "can", which likely doesn't translate easily. https://codereview.adblockplus.org/29332455/diff/29332456/src/plugin/PluginTa... src/plugin/PluginTabBase.h:81: * ABP only intercepts protocols "http:" and "https:", On 2015/12/08 20:20:19, sergei wrote: > I would rather move it into the method body because it explains the magic > strings used there. Done. https://codereview.adblockplus.org/29332455/diff/29332456/src/plugin/PluginTa... src/plugin/PluginTabBase.h:82: * so it makes no sense to offer the user an option to block content from other protocols. On 2015/12/08 20:20:19, sergei wrote: > I bit unusual formatting with two spaces gap. I rewrote this piece of the comment entirely.
On 2015/12/09 12:48:35, Eric wrote: > Regardless of what choice we make, there's a possible follow-up change set to > this one. (A) If we disable, we'd ideally have a tooltip explaining this menu > item, with text for each of its three display states. In addition to the code, > that would also require translation strings. (B) If we remove, we need code to > do it. > > So I'd rather not have a big discussion about the UI design right now. ... > Personally, I think in this case that disabling is better than removing. I agree here. https://codereview.adblockplus.org/29332455/diff/29332480/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29332455/diff/29332480/src/plugin/PluginCl... src/plugin/PluginClass.cpp:1139: fmii.fState = MFS_UNCHECKED | MFS_DISABLED; If it is not possible to disable on site, it also means that we are already disabled there (we don't intercept non-http(s) links). So wouldn't it make more sense to make this: fmii.fState = MFS_CHECKED | MFS_DISABLED;?
To be clear, I'd like to fix these defects first and deal with any subsequent UI
changes later. I've created #3428 so that we don't have to discuss all these
details here. If the present UI is good enough for now, we can move forward.
Decide the UI details for non-blockable URL's
https://issues.adblockplus.org/ticket/3428
https://codereview.adblockplus.org/29332455/diff/29332480/src/plugin/PluginCl...
File src/plugin/PluginClass.cpp (right):
https://codereview.adblockplus.org/29332455/diff/29332480/src/plugin/PluginCl...
src/plugin/PluginClass.cpp:1139: fmii.fState = MFS_UNCHECKED | MFS_DISABLED;
On 2015/12/17 12:59:38, Oleksandr wrote:
> If it is not possible to disable on site, it also means that we are already
> disabled there (we don't intercept non-http(s) links). So wouldn't it make
more
> sense to make this: fmii.fState = MFS_CHECKED | MFS_DISABLED;?
The meaning of the check mark to me is "there is a custom override rule to
disable on this site". Since we're disabling these URL's by category, there's no
user override at play.
Also, a check mark generally indicates something you can tick on and tick off.
Putting up a check mark that the user can't uncheck breaks an ordinary
expectation of what those mean.
Fair enough. LGTM.
On 2015/12/17 13:40:39, Eric wrote: > To be clear, I'd like to fix these defects first and deal with any subsequent UI > changes later. I've created #3428 so that we don't have to discuss all these > details here. If the present UI is good enough for now, we can move forward. > > Decide the UI details for non-blockable URL's > https://issues.adblockplus.org/ticket/3428 It seems that #1596 should depend on #3428. On 2015/12/17 12:59:38, Oleksandr wrote: > On 2015/12/09 12:48:35, Eric wrote: > > > Regardless of what choice we make, there's a possible follow-up change set to > > this one. (A) If we disable, we'd ideally have a tooltip explaining this menu > > item, with text for each of its three display states. In addition to the code, > > that would also require translation strings. (B) If we remove, we need code to > > do it. > > > > So I'd rather not have a big discussion about the UI design right now. > ... > > > Personally, I think in this case that disabling is better than removing. > > I agree here. I disagree because removing of the item seems to be consistent with UX in Chrome and Firefox (commented in #3428). Just in case, it's not so difficult to remove the item, e.g. so following should work if (GetTab()->IsPossibleToDisableOnSite()) >{ > ctext = dictionary->Lookup("menu", "menu-disable-on-site"); > // Is domain in white list? > ReplaceString(ctext, L"?1?", client->GetHostFromUrl(url)); > if (client->IsWhitelistedUrl(GetTab()->GetDocumentUrl())) > { > fmii.fState = MFS_CHECKED | MFS_ENABLED; > } > else > { > fmii.fState = MFS_UNCHECKED | MFS_ENABLED; > } > fmii.fMask = MIIM_STRING | MIIM_STATE; > fmii.dwTypeData = const_cast<LPWSTR>(ctext.c_str()); > fmii.cch = static_cast<UINT>(ctext.size()); > > ::SetMenuItemInfoW(hMenu, ID_MENU_DISABLE_ON_SITE, FALSE, &fmii); >} else { ::DeleteMenu(hMenu, ID_MENU_DISABLE_ON_SITE, MF_BYCOMMAND); } BTW, I've filed yet one ticket related to the issue https://issues.adblockplus.org/ticket/3441. On 2015/12/17 14:18:17, Oleksandr wrote: > Fair enough. LGTM. Yes, it seems to work but I think the scale of the fix (amount of changes) is not so big that we should accept it now and make the further changes later. Why not to firstly think and then submit the proper solution without returning to the issue again? https://codereview.adblockplus.org/29332455/diff/29332480/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29332455/diff/29332480/src/plugin/PluginCl... src/plugin/PluginClass.cpp:1139: fmii.fState = MFS_UNCHECKED | MFS_DISABLED; On 2015/12/17 13:40:39, Eric wrote: > On 2015/12/17 12:59:38, Oleksandr wrote: > > If it is not possible to disable on site, it also means that we are already > > disabled there (we don't intercept non-http(s) links). So wouldn't it make > more > > sense to make this: fmii.fState = MFS_CHECKED | MFS_DISABLED;? I agree, it should be rather checked. > > The meaning of the check mark to me is "there is a custom override rule to > disable on this site". Since we're disabling these URL's by category, there's no > user override at play. Why do you think so? The meaning of this check mark is that ABP is disabled on this particular site and there is no any connection between the way or reason ABP is disabled, whether it's a custom override rule or a rule from some subscription or it's hard-coded in the plugin. Currently the user can interpret it as ABP does work on such pages and cannot be disabled. > > Also, a check mark generally indicates something you can tick on and tick off. > Putting up a check mark that the user can't uncheck breaks an ordinary > expectation of what those mean. So, not putting up a check mark that user cannot check also breaks the expectation, doesn't it? Actually, it does not break the expectation because it's well known fact that control can have a disabled state which means the user is not allowed to change it.
On 2015/12/21 10:26:23, sergei wrote: > I disagree because removing of the item seems to be consistent with UX in Chrome > and Firefox (commented in #3428). The whole reason I created #3428 is to move the UI design discussion *out* of this issue and *into* another one. Please don't drag it back here. > Just in case, it's not so difficult to remove the item, e.g. so following should > work It's not difficult, no. It's also not germane to the present change set, which is about fixing two defects by introducing 'IsPossibleToDisableOnSite()'. You are perfectly welcome to submit a change set of your own for review after we get the present one committed. > Yes, it seems to work but I think the scale of the fix (amount of changes) is > not so big that we should accept it now and make the further changes later. It is indeed not so big, which means it should be very little work for you to introduce this change later, after we commit the present change set. > Why > not to firstly think and then submit the proper solution without returning to > the issue again? I disagree that your suggestion is the "proper solution". Therefore I created #3428. You have no open concerns with 'IsPossibleToDisableOnSite()'; I've addressed all of them you've raised. Apparently we agree on that. We can move forward on what we agree on and discuss elsewhere what we don't agree on. https://codereview.adblockplus.org/29332455/diff/29332480/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29332455/diff/29332480/src/plugin/PluginCl... src/plugin/PluginClass.cpp:1139: fmii.fState = MFS_UNCHECKED | MFS_DISABLED; Please feel free to bring up these points as comments to #3428, where the UI discussion now belongs.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
