change active layer icon in pcbnew changes all the time

[Oleg Endo]

The icon for "change active layer" in pcbnew differs when it is launched for the first time.

When kicad is freshly started, pcbnew opened from the main window, I get icon 1. After closing pcbnew and starting it again, I get icon 2.

Application: KiCad
Version: 6.0.0-unknown-2af41e8~86~ubuntu18.04.1, release build
Libraries:
    wxWidgets 3.0.4
    libcurl/7.58.0 OpenSSL/1.1.1 zlib/1.2.11 libidn2/2.0.4 libpsl/0.19.1 (+libidn2/2.0.4) nghttp2/1.30.0 librtmp/2.3
Platform: Linux 4.15.0-54-generic x86_64, 64 bit, Little endian, wxGTK
Build Info:
    wxWidgets: 3.0.4 (wchar_t,wx containers,compatible with 2.8) GTK+ 3.22
    Boost: 1.65.1
    OpenCASCADE Community Edition: 6.9.1
    Curl: 7.58.0
    Compiler: GCC 7.4.0 with C++ ABI 1011

Build settings:
    KICAD_SCRIPTING=ON
    KICAD_SCRIPTING_MODULES=ON
    KICAD_SCRIPTING_PYTHON3=ON
    KICAD_SCRIPTING_WXPYTHON=ON
    KICAD_SCRIPTING_WXPYTHON_PHOENIX=ON
    KICAD_SCRIPTING_ACTION_MENU=ON
    BUILD_GITHUB_PLUGIN=ON
    KICAD_USE_OCE=ON
    KICAD_USE_OCC=OFF
    KICAD_SPICE=ON

[Ian McInerney (imcinerney)]

There is an inconsistency with the icon in 5.1 as well:

Icon 1 is shown in the toolbar, but the menu item in the tools menu to open the same dialog is using icon 2.

[Jeff Young (jeyjey)]

The menu icon is supposed to be different. We use a special icon that's rendered with the actual layer colors in the toolbar, but not in the menu.

[Jeff Young (jeyjey)]

The wrong icon in the toolbar on second launch doesn't reproduce on OSX, but I suspect it's because the LLDB compiler initialized stack memory to 0 and GCC doesn't. So I've merged a fix that might address it.

I'll wait for confirmation before cherry-picking it to 5.1.

[KiCad Janitor (kicad-janitor)]

Fixed in revision ee1be14b62334b4bb0782a04dc9d7b1d4b8bfce6
https://git.launchpad.net/kicad/patch/?id=ee1be14b62334b4bb0782a04dc9d7b1d4b8bfce6

[Michael Kavanagh (michaelkavanagh)]

Hmm, no luck unfortunately, I can still reproduce this. Interestingly, on the second launch if you change the layer pair in the dialog, the correct icon reappears in the toolbar.

Application: Pcbnew
Version: (5.1.0-1383-gee1be14b6), debug build
Libraries:
    wxWidgets 3.0.4
    libcurl/7.54.0 LibreSSL/2.6.5 zlib/1.2.11 nghttp2/1.24.1
Platform: Mac OS X (Darwin 18.6.0 x86_64), 64 bit, Little endian, wxMac
Build Info:
    wxWidgets: 3.0.4 (wchar_t,STL containers,compatible with 2.8)
    Boost: 1.70.0
    OpenCASCADE Community Edition: 6.9.1
    Curl: 7.54.0
    Compiler: Clang 10.0.1 with C++ ABI 1002

Build settings:
    KICAD_SCRIPTING=OFF
    KICAD_SCRIPTING_MODULES=OFF
    KICAD_SCRIPTING_PYTHON3=OFF
    KICAD_SCRIPTING_WXPYTHON=OFF
    KICAD_SCRIPTING_WXPYTHON_PHOENIX=OFF
    KICAD_SCRIPTING_ACTION_MENU=OFF
    BUILD_GITHUB_PLUGIN=ON
    KICAD_USE_OCE=ON
    KICAD_USE_OCC=OFF
    KICAD_SPICE=ON

[Jeff Young (jeyjey)]

@Michael, I've merged another attempt. Let me know if it works when you get a chance.

[Ian McInerney (imcinerney)]

@Jeff, this actually isn't due to the lack of static variable initialization. What seems to happen is that the static variables maintain their value across the pcbnew instances on Linux (so the .so is probably not unloaded). This means the checks saying if the bitmap should be rebuilt say not to when pcbnew is run the second time.

To fix this, we just need to force the bitmap to be rebuilt on menu creation. See the attached patch.

[Ian McInerney (imcinerney)]

@Jeff, I just tried the fix you committed the second time around and it still doesn't give the right bitmap on the second opening. The problem is still probably the use of the static variable to say if it was initialized.

Note that my patch was actually made from your first attempt (before your second), and it fixed the issue on my Linux machine.

[Jeff Young (jeyjey)]

Wow, that's kind of scary. I wonder what else that breaks...

Thanks for the patch!

[Ian McInerney (imcinerney)]

Yea, static variables and C++ are kind of scary sometimes.

The current system is probably not going to be multi-instance safe either. The static variables will be shared between every instance of PCB_EDIT_FRAME, so if someone has 1 instance of pcbnew with top/bottom selected and another with inner1/inner2 selected, then the toolbar updates will constantly regenerate the icons because each update will keep switching the value in the static variable to their values. This kind of information should probably be moved into a member struct containing the previous colors so that it stays with the object (also, that would eliminate the need to have this boolean argument since we can initialize them to default when constructed).

The use of statics currently to hold dialog settings is probably fine since those shouldn't break anything being shared across instances, but we need to be careful about other uses.

[KiCad Janitor (kicad-janitor)]

Fixed in revision b70df18939ba382eb6940f7ecc78e9cb54c13852
https://git.launchpad.net/kicad/patch/?id=b70df18939ba382eb6940f7ecc78e9cb54c13852

[Jeff Young (jeyjey)]

Cherry-picking didn't work out because of other changes, so I implemented another fix.

[Oleg Endo (oleg.endo)]

Which icon (see my screenshot above) is supposed to be displayed now actually?

[Jeff Young (jeyjey)]

Icon #1 (although the colours will be modified to match the two layers currently chosen).

[Oleg Endo (oleg.endo)]

OK, thanks. Seems to be working now.