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:
(http://i.imgur.com/81JBVAT.png)
Second icon:
(http://i.imgur.com/VPWeTpv.png)
Third icon:
(http://i.imgur.com/g6KGczR.png)
Thanks.
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. :)
Quote from: ollydbg on April 08, 2013, 11:30:32 AM
I believe the code above have some issue. :)
Fix it then...
When the page changes, in the function void EditorConfigurationDlg::OnPageChanged(wxListbookEvent& event), there is some code snippet called, which "de-select" the icon.
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.
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?
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();
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?!
Probably something like this:
(http://i.imgur.com/g6KGczR.png)
Quote from: oBFusCATed on April 08, 2013, 08:39:18 PM
Probably something like this:
This doesn't look "ruined" but "as expected" to me... at least from a Windows point of view.
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).
Quote from: jens on April 08, 2013, 10:56:00 PM
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?
Ok, can you test the patch below:
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.
Quote from: ollydbg on April 09, 2013, 03:25:12 AM
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.
Quote from: oBFusCATed on April 09, 2013, 09:24:46 AM
Quote from: ollydbg on April 09, 2013, 03:25:12 AM
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.
Quote from: ollydbg on April 09, 2013, 09:27:22 AM
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.
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.
Quote from: MortenMacFly on April 09, 2013, 09:59:03 PM
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?
Quote from: ollydbg on April 10, 2013, 04:20:25 PM
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.
Quote from: MortenMacFly on April 10, 2013, 05:38:48 PM
Quote from: ollydbg on April 10, 2013, 04:20:25 PM
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:
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.
(http://i683.photobucket.com/albums/vv194/ollydbg_cb/2013-04-12092953_zps268e3d36.png)
Quote from: ollydbg on April 12, 2013, 03:29:15 AM
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.
(http://i683.photobucket.com/albums/vv194/ollydbg_cb/2013-04-12092953_zps268e3d36.png)
Even better - go ahead then! :-)
Done in rev8985, thanks everyone.