Configure dialog icon issue when clicked

[ollydbg]

Hi, I have three screen shots below, the first one is that a plugin icon is not selected, so the gray icon is shown, the second one is one mouse click, so that a color icon is shown, the third one is when after another mouse click, the icon show in a shadow. My question is: Does the third one is the expected behavior? I see no difference between the second and the third. But I'd say that an color icon with shadow is not very good, what do you think?

First icon:

Second icon:

Third icon:


Thanks.

[ollydbg]

Code

void EditorConfigurationDlg::UpdateListbookImages()
{
    wxListbook* lb = XRCCTRL(*this, "nbMain", wxListbook);
    int sel = lb->GetSelection();
    // set page images according to their on/off status
    for (size_t i = 0; i < IMAGES_COUNT + m_PluginPanels.GetCount(); ++i)
    {
        lb->SetPageImage(i, (i * 2) + (sel == (int)i ? 0 : 1));
    }

    // the selection colour is ruining the on/off effect,
    // so make sure no item is selected ;)
    // (only if we have icons showing)
    if (GetSettingsIconsStyle(lb->GetListView()) != sisNoIcons)
        lb->GetListView()->Select(sel, false);

    // update the page title
    wxString label = lb->GetPageText(sel);
    // replace any stray & with && because label makes it an underscore
    while (label.Replace(_T(" & "), _T(" && ")))
        ;
    XRCCTRL(*this, "lblBigTitle", wxStaticText)->SetLabel(label);
    XRCCTRL(*this, "pnlTitleInfo", wxPanel)->Layout();
}

void EditorConfigurationDlg::OnPageChanged(wxListbookEvent& event)
{
    // update only on real change, not on dialog creation
    if (event.GetOldSelection() != -1 && event.GetSelection() != -1)
    {
        UpdateListbookImages();
    }
}

I believe the code above have some issue.

[oBFusCATed]

I believe the code above have some issue.

Fix it then...

[ollydbg]

When the page changes, in the function void EditorConfigurationDlg::OnPageChanged(wxListbookEvent& event), there is some code snippet called, which "de-select" the icon.

Code

    if (GetSettingsIconsStyle(lb->GetListView()) != sisNoIcons)
        lb->GetListView()->Select(sel, false);

But when the user click on the icon again, no On Page changed event will be sent, so the icon is select by default(so you see the third screen shot). Is there any way to change this? I guess it should be done in EVT_LIST_ITEM_SELECTED(id, func) of the wxListCtrl class, but is that possible? I have no idea.

[ollydbg]

This is the patch to fix such issue, it just hook the list item select event and deselect it to avoid showing the shadow, any comments?

Code

Index: editorconfigurationdlg.cpp
===================================================================
--- editorconfigurationdlg.cpp	(revision 8972)
+++ editorconfigurationdlg.cpp	(working copy)
@@ -282,6 +282,7 @@
     lb->AssignImageList(images);
     int sel = Manager::Get()->GetConfigManager(_T("app"))->ReadInt(_T("/environment/settings_size"), 0);
     SetSettingsIconsStyle(lb->GetListView(), (SettingsIconsStyle)sel);
+    lb->GetListView()->Connect(wxEVT_COMMAND_LIST_ITEM_SELECTED, wxListEventHandler( EditorConfigurationDlg::ListItemSelected), NULL, this);
 
     // add all plugins configuration panels
     AddPluginPanels();
@@ -299,6 +300,9 @@
 
 EditorConfigurationDlg::~EditorConfigurationDlg()
 {
+    wxListbook* lb = XRCCTRL(*this, "nbMain", wxListbook);
+    lb->GetListView()->Disconnect(wxEVT_COMMAND_LIST_ITEM_SELECTED, wxListEventHandler( EditorConfigurationDlg::ListItemSelected), NULL, this);
+
     if (m_Theme)
         delete m_Theme;
 
@@ -361,6 +365,14 @@
     XRCCTRL(*this, "pnlTitleInfo", wxPanel)->Layout();
 }
 
+void EditorConfigurationDlg::ListItemSelected(wxListEvent& event)
+{
+    wxListbook* lb = XRCCTRL(*this, "nbMain", wxListbook);
+    int sel = lb->GetSelection();
+    lb->GetListView()->Select(sel, false);
+    event.Skip();
+}
+
 void EditorConfigurationDlg::OnPageChanged(wxListbookEvent& event)
 {
     // update only on real change, not on dialog creation
Index: editorconfigurationdlg.h
===================================================================
--- editorconfigurationdlg.h	(revision 8972)
+++ editorconfigurationdlg.h	(working copy)
@@ -50,6 +50,7 @@
         void EndModal(int retCode);
     private:
         void OnPageChanged(wxListbookEvent& event);
+        void ListItemSelected(wxListEvent& event);
         void AddPluginPanels();
         void UpdateListbookImages();
         void CreateColoursSample();

[MortenMacFly]

I don't understand the root issue: What happens if no such hack is implemented at all? How does a "ruined" icon look like? This maybe just be a "hack" from earlier wx2.6 versions, maybe, which we can simply remove...

Implementing a hack for a hack sounds a bit strange to me... although if it is really needed... why not?!

[oBFusCATed]

Probably something like this:

[MortenMacFly]

Probably something like this:

This doesn't look "ruined" but "as expected" to me... at least from a Windows point of view.

[Jenna]

On linux the icon is not selected, just the text, but as far as I see the default behaviour on windows is to select the icon and the text (e.g. the security settings in internet options of IE).
It is probably be the best to remove the deselect-hack to get the native behaviour (at least for windows and linux).

[ollydbg]

On linux the icon is not selected, just the text, but as far as I see the default behaviour on windows is to select the icon and the text (e.g. the security settings in internet options of IE).
It is probably be the best to remove the deselect-hack to get the native behaviour (at least for windows and linux).

Hi, jens, I agree with you, let's use the native behaviour on Windows. I just tested IE security settings dialog, wxListCtrl sample, Windows Explorer with icon+text view and C::B project wizard, all the behaviour are the same: both icon and text are selected and become a littler darker.

So, the status in my second screenshot of original post should be removed. Wait, does this means that all the xxxx-off.png image files are not need? Because we now have two status: the color image, and the "ruined" color image. If that is done, what's the behaviour under Linux? Any comments?

[ollydbg]

Ok, can you test the patch below:

Code

Index: E:/code/cb/cb_trunk_sf/src/src/editorconfigurationdlg.cpp
===================================================================
--- E:/code/cb/cb_trunk_sf/src/src/editorconfigurationdlg.cpp	(revision 8972)
+++ E:/code/cb/cb_trunk_sf/src/src/editorconfigurationdlg.cpp	(working copy)
@@ -275,8 +275,6 @@
     {
         bmp = cbLoadBitmap(base + base_imgs[i] + _T(".png"), wxBITMAP_TYPE_PNG);
         images->Add(bmp);
-        bmp = cbLoadBitmap(base + base_imgs[i] + _T("-off.png"), wxBITMAP_TYPE_PNG);
-        images->Add(bmp);
     }
     wxListbook* lb = XRCCTRL(*this, "nbMain", wxListbook);
     lb->AssignImageList(images);
@@ -286,6 +284,8 @@
     // add all plugins configuration panels
     AddPluginPanels();
 
+    UpdateListbookImages();
+
     // the following code causes a huge dialog to be created with wx2.8.4
     // commenting it out fixes the problem (along with some XRC surgery)
     // if this causes problems with earlier wx versions we might need to
@@ -309,7 +309,7 @@
 void EditorConfigurationDlg::AddPluginPanels()
 {
     const wxString base = _T("images/settings/");
-    const wxString noimg = _T("images/settings/generic-plugin");
+    const wxString noimg = _T("images/settings/generic-plugin"); //for those plugins who does not supply icons
 
     wxListbook* lb = XRCCTRL(*this, "nbMain", wxListbook);
     // get all configuration panels which are about the editor.
@@ -321,37 +321,26 @@
         panel->SetParentDialog(this);
         lb->AddPage(panel, panel->GetTitle());
 
-        wxString onFile = ConfigManager::LocateDataFile(base + panel->GetBitmapBaseName() + _T(".png"), sdDataGlobal | sdDataUser);
-        if (onFile.IsEmpty())
-            onFile = ConfigManager::LocateDataFile(noimg + _T(".png"), sdDataGlobal | sdDataUser);
-        wxString offFile = ConfigManager::LocateDataFile(base + panel->GetBitmapBaseName() + _T("-off.png"), sdDataGlobal | sdDataUser);
-        if (offFile.IsEmpty())
-            offFile = ConfigManager::LocateDataFile(noimg + _T("-off.png"), sdDataGlobal | sdDataUser);
+        wxString iconFile = ConfigManager::LocateDataFile(base + panel->GetBitmapBaseName() + _T(".png"), sdDataGlobal | sdDataUser);
+        if (iconFile.IsEmpty())
+            iconFile = ConfigManager::LocateDataFile(noimg + _T(".png"), sdDataGlobal | sdDataUser);
 
-        lb->GetImageList()->Add(cbLoadBitmap(onFile));
-        lb->GetImageList()->Add(cbLoadBitmap(offFile));
-        lb->SetPageImage(lb->GetPageCount() - 1, lb->GetImageList()->GetImageCount() - 2);
+        lb->GetImageList()->Add(cbLoadBitmap(iconFile));
+
+        lb->SetPageImage(lb->GetPageCount() - 1, lb->GetImageList()->GetImageCount() - 1);
     }
-
-    UpdateListbookImages();
 }
 
 void EditorConfigurationDlg::UpdateListbookImages()
 {
     wxListbook* lb = XRCCTRL(*this, "nbMain", wxListbook);
     int sel = lb->GetSelection();
-    // set page images according to their on/off status
+    // set page images
     for (size_t i = 0; i < IMAGES_COUNT + m_PluginPanels.GetCount(); ++i)
     {
-        lb->SetPageImage(i, (i * 2) + (sel == (int)i ? 0 : 1));
+        lb->SetPageImage(i, i);
     }
 
-    // the selection colour is ruining the on/off effect,
-    // so make sure no item is selected ;)
-    // (only if we have icons showing)
-    if (GetSettingsIconsStyle(lb->GetListView()) != sisNoIcons)
-        lb->GetListView()->Select(sel, false);
-
     // update the page title
     wxString label = lb->GetPageText(sel);
     // replace any stray & with && because label makes it an underscore

I just remove all the off.png related code.

The further improvement as I see is that: Do not call the UpdateListbookImages() in the OnPageChanged Event handler, I think we can create a simple function like: UpdateListbookTitles() instead.

[oBFusCATed]

I just remove all the off.png related code.

Please before you commit test it on linux. Probably this is a linux hack to make icons different.

[ollydbg]

I just remove all the off.png related code.

Please before you commit test it on linux. Probably this is a linux hack to make icons different.

OK, no hurry, I'm waiting for all your comments especially on Linux testing result.

[oBFusCATed]

OK, no hurry, I'm waiting for all your comments especially on Linux testing result.

I had no intention to test it.
I've just said that probably removing the off images is not good idea on linux.
Probably because there is no automatic highlight in wxGTK.

[MortenMacFly]

I dont see an important reason to remove the black and white Images. Just the hack in a First place would be fine and does Not affect 3rd Party plugins at all.

[ollydbg]

Just the hack in a First place would be fine and does Not affect 3rd Party plugins at all.

Hi, Morten, I don't understand this sentence, what does the "hack in a First place" mean?

[MortenMacFly]

Hi, Morten, I don't understand this sentence, what does the "hack in a First place" mean?

I mean as a first commit it would be fair enough to just remove the hack with the de-selection. Not removing logic with changing images from coloured to b/w.

[ollydbg]

Hi, Morten, I don't understand this sentence, what does the "hack in a First place" mean?

I mean as a first commit it would be fair enough to just remove the hack with the de-selection. Not removing logic with changing images from coloured to b/w.

Ok, follow your advice, the patch becomes a lot simple, just remove the hack of de-selection. See below:

Code

Index: E:/code/cb/cb_trunk_sf/src/src/editorconfigurationdlg.cpp
===================================================================
--- E:/code/cb/cb_trunk_sf/src/src/editorconfigurationdlg.cpp	(revision 8972)
+++ E:/code/cb/cb_trunk_sf/src/src/editorconfigurationdlg.cpp	(working copy)
@@ -309,7 +309,7 @@
 void EditorConfigurationDlg::AddPluginPanels()
 {
     const wxString base = _T("images/settings/");
-    const wxString noimg = _T("images/settings/generic-plugin");
+    const wxString noimg = _T("images/settings/generic-plugin"); //for those plugins who does not supply icons
 
     wxListbook* lb = XRCCTRL(*this, "nbMain", wxListbook);
     // get all configuration panels which are about the editor.
@@ -346,12 +346,6 @@
         lb->SetPageImage(i, (i * 2) + (sel == (int)i ? 0 : 1));
     }
 
-    // the selection colour is ruining the on/off effect,
-    // so make sure no item is selected ;)
-    // (only if we have icons showing)
-    if (GetSettingsIconsStyle(lb->GetListView()) != sisNoIcons)
-        lb->GetListView()->Select(sel, false);
-
     // update the page title
     wxString label = lb->GetPageText(sel);
     // replace any stray & with && because label makes it an underscore

BTW: I see the "selection ruining " only affect on the non-transparent part of the icon. See the image shot below, the colorful icon of codecompletion is selected under Windows, but not the full icon has ruined compared with the third icon screen shot in my original post.

[MortenMacFly]

BTW: I see the "selection ruining " only affect on the non-transparent part of the icon. See the image shot below, the colorful icon of codecompletion is selected under Windows, but not the full icon has ruined compared with the third icon screen shot in my original post.

Even better - go ahead then! :-)

[ollydbg]

Done in rev8985, thanks everyone.