Restore Main_no_icons2 behavior for inline subitems

[among-us-official]

Fix regression caused by b89aaed.
#1088 (comment)

The original commit fixed a bug where the icons would not be generated for the second column's icon size. However, the bugged behavior had masked a discrepancy between the icons generated for the inlined submenus (inserted as buttons) in CMenuContainer::AddStandardItems and the drawing settings for them in MenuSkin::LoadSkin. Here are the snippets in order:

Open-Shell-Menu/Src/StartMenu/StartMenuDLL/MenuContainer.cpp

Lines 1275 to 1305 in e6b33a7

	CItemManager::TIconSizeType iconSizeType;
	int refreshFlags;
	if (bSearchProvider7 || m_bSubMenu)
	{
	iconSizeType=CItemManager::ICON_SIZE_TYPE_SMALL;
	refreshFlags=CItemManager::INFO_SMALL_ICON;
	}
	else if (s_bWin7Style)
	{
	iconSizeType=CItemManager::ICON_SIZE_TYPE_EXTRA_LARGE;
	refreshFlags=CItemManager::INFO_EXTRA_LARGE_ICON;
	}
	else if (mainIconSize==MenuSkin::ICON_SIZE_LARGE)
	{
	iconSizeType=CItemManager::ICON_SIZE_TYPE_LARGE;
	refreshFlags=CItemManager::INFO_LARGE_ICON;
	}
	else if (mainIconSize==MenuSkin::ICON_SIZE_SMALL)
	{
	iconSizeType=CItemManager::ICON_SIZE_TYPE_SMALL;
	refreshFlags=CItemManager::INFO_SMALL_ICON;
	}
	else
	{
	iconSizeType=CItemManager::ICON_SIZE_TYPE_SMALL;
	refreshFlags=0;
	}
	if (pStdItem->link)
	refreshFlags|=CItemManager::INFO_LINK|CItemManager::INFO_METRO;
	if ((refreshFlags&CItemManager::INFO_ICON)==0)
	item.pItemInfo=NULL;

Open-Shell-Menu/Src/StartMenu/StartMenuDLL/SkinManager.cpp

Lines 2932 to 2933 in e6b33a7

	case COLUMN2_INLINE:
	settings.iconSize=Main2_icon_size!=ICON_SIZE_NONE?Main2_icon_size:Main_icon_size;

Since previously mainIconSize was always equal to Main_icon_size (first column's icons), the correct icons were created for the subitems. This workaround restores the old, more correct behavior by saving mainIconSize and restoring it after the related code is over.

Maybe a more permanent solution is to refactor the icon code to run after AddStandardItems, when we know about the icon sizes for each item?

[AppVeyorBot]

✅ Build Open-Shell-Menu 4.4.180-judrkdjd completed (commit 125b522da9 by @among-us-official)

[blackcrack]

a thank you button.. 🤔 or an 🤗 button would nice at the reacting 😊

[ge0rdi]

@bonzibudd

Could you, please, verify that Insert sub-items as buttons option works correctly for you using build from this PR?
Note that you should back-up your setting prior to using PR build and then also remove it after testing.

[bonzibudd]

Yes, I believe it is working normally now!

[ge0rdi]

@among-us-official

I was rather thinking about change like this (applicable on master):

diff --git a/Src/StartMenu/StartMenuDLL/MenuContainer.cpp b/Src/StartMenu/StartMenuDLL/MenuContainer.cpp
index b6d6967..a859ab2 100644
--- a/Src/StartMenu/StartMenuDLL/MenuContainer.cpp
+++ b/Src/StartMenu/StartMenuDLL/MenuContainer.cpp
@@ -1071,7 +1071,7 @@ void CMenuContainer::AddStandardItems( void )
 		const StdMenuItem *pInlineParent=NULL;
 		int searchProviderIndex=-1;
 		m_SearchProvidersCount=0;
-		MenuSkin::TIconSize mainIconSize=s_Skin.Main_icon_size;
+		bool bSecondColumn = false;
 		for (const StdMenuItem *pStdItem=m_pStdItem;;pStdItem++)
 		{
 			if (pStdItem->id==MENU_LAST)
@@ -1089,9 +1089,8 @@ void CMenuContainer::AddStandardItems( void )
 			if (m_bSubMenu && pStdItem->id==s_ShutdownCommand)
 				continue;
 
-			const bool bTwoColumns = (!m_bSubMenu && s_Skin.TwoColumns);
-			if (pStdItem->id==MENU_COLUMN_BREAK && bTwoColumns)
-				mainIconSize=s_Skin.Main2_icon_size;
+			if (pStdItem->id==MENU_COLUMN_BREAK && !m_bSubMenu && s_Skin.TwoColumns)
+				bSecondColumn = true;
 
 			int stdOptions=GetStdOptions(pStdItem->id);
 			if (!(stdOptions&MENU_ENABLED)) continue;
@@ -1272,6 +1271,10 @@ void CMenuContainer::AddStandardItems( void )
 			item.bSplit=item.bFolder && (item.pStdItem->settings&StdMenuItem::MENU_SPLIT_BUTTON)!=0;
 
 			// get icon
+			MenuSkin::TIconSize mainIconSize = s_Skin.Main_icon_size;
+			if (bSecondColumn && !item.bInline)
+				mainIconSize = s_Skin.Main2_icon_size;
+
 			CItemManager::TIconSizeType iconSizeType;
 			int refreshFlags;
 			if (bSearchProvider7 || m_bSubMenu)

It should be simpler (no need for weird old value storing/restoring) and hopefully clear too.
What do you think?

[among-us-official]

That's clearer, yeah.

[ge0rdi]

Oki. Could you modify the PR then? And test whether it works as expected?

[AppVeyorBot]

✅ Build Open-Shell-Menu 4.4.180-bwbrwaai completed (commit b9e221f01d by @among-us-official)

[AppVeyorBot]

✅ Build Open-Shell-Menu 4.4.180-fupxgged completed (commit b091048660 by @among-us-official)

[among-us-official]

@ge0rdi Tested it, works.

[ge0rdi]

@among-us-official
Thank you.

@bonzibudd
Could you, please, test once more with latest build?

[bonzibudd]

Just tried with the newer build, while my default config with the sub-items works fine, using Main2_icon_size=small with sub-items results in corrupted icons.

Initial commit works fine:

Any thoughts?

[ge0rdi]

@bonzibudd
Could you share the skin you are using and your settings?

[ge0rdi]

Also, it's quite weird that first line looks good and only subsequent are corrupted.
Maybe some caching issue?
Could you try Settings -> Backup -> Clear cached information ?

[bonzibudd]

Small Main2 w subitems.zip

Skin is just a quick edit to my existing skin that adds main2_icon_size=small to the start of the script. The XML adds the sub-items as they are shown in the picture. Interestingly, when I clear the cache, the top row also corrupts.

[among-us-official]

@bonzibudd Thanks so much for testing!! Can't believe that in less than a day I've forgotten what that MenuSkin::ICON_SIZE_NONE check was doing. I've put it back in, it should now work correctly.

[AppVeyorBot]

✅ Build Open-Shell-Menu 4.4.180-murcoman completed (commit 207633dc95 by @among-us-official)

[bonzibudd]

@among-us-official Yes, it seems to be working again. Thank you for your help with this!

[ge0rdi]

Can't believe that in less than a day I've forgotten what that MenuSkin::ICON_SIZE_NONE check was doing. I've put it back in, it should now work correctly.

You could have explained why it is needed like that :)
But I guess I have figured it out too.

It is clear from sking manager's code that Main2_icon_size is used for second column except when the column is inline and Main2_icon_size is none. In such case Main_icon_size is used instead.

Now the code in menu container matches the one in skin manager. So hopefully it will be final fix :)

[ge0rdi]

I have tested the change as well and it looks that everything works as expected.
@among-us-official @bonzibudd Thank you guys.