Odd bug in plugin menu entries

matterhorn103 · December 11, 2024, 2:20pm

I believe this to be a bug with Avogadro

Environment Information

Avogadro version: 1.99 nightly AppImage and beta Flatpak
Operating system and version: Linux (openSUSE Tumbleweed)

Expected Behavior

Menu entry for avo_xtb plugin shows submenu on hover

Actual Behavior

Nothing happens on hover

Steps to Reproduce

Install presumably any plugin that adds multiple commands in a sub-menu, such as avo_xtb

Bit of a weird one this one, in that the beta Flatpak (Qt6) continues to have the correct behaviour, while the nightly^[1] AppImage (Qt5) and local builds (both Qt5 and Qt6) built from the latest^[1:1] code all show the bug.

The Flatpak is a few commits behind master, but checking out the same commit of avogadrolibs locally (42c349a) and building from that doesn’t seem to fix it.

Meanwhile on Fedora neither the AppImage nor the Flatpak show the issue.

I can’t really work out why this should be happening and I can’t see what would have changed to cause it to start happening, happy to try or check out anything you might suggest.

Yesterday’s build/code (2024-12-10) ↩︎ ↩︎

matterhorn103 · January 12, 2025, 11:21pm

Just to come back to this with some pictures to demonstrate…

I checked and have confirmed that it’s definitely not an issue specific to the avo_xtb plugin.

Here I’m using a very basic command script:

import argparse
import json

if __name__ == "__main__":
    parser = argparse.ArgumentParser()
    parser.add_argument("--debug", action="store_true")
    parser.add_argument("--print-options", action="store_true")
    parser.add_argument("--run-command", action="store_true")
    parser.add_argument("--display-name", action="store_true")
    parser.add_argument("--lang", nargs="?", default="en")
    parser.add_argument("--menu-path", action="store_true")
    args = parser.parse_args()

    if args.print_options:
        options = {}
        print(json.dumps(options))
        
    if args.display_name:
        print("Hello World")

    if args.menu_path:
        print("Extensions{100}")

    if args.run_command:
        output = {"message": "Hello, World!"}
        print(json.dumps(output))

In my native self-compiled builds (compiled just now from the latest source code) the menu entry doesn’t show up at all:

If I change the menu path to

    if args.menu_path:
        print("Extensions|TestMenu{100}")

the result is an entry for TestMenu in the Extensions menu but hovering over it does not reveal the submenu as it is supposed to:

By comparison, normal submenus look fine:

The behaviour is the same regardless of Qt5/Qt6 or whether the locale is “system” or US English.

As mentioned, this is all on OpenSUSE Tumbleweed, where the AppImage is also broken, but the Flatpak works fine, as do all versions on Fedora.

matterhorn103 · January 12, 2025, 11:28pm

One strange thing is that if I navigate the menus with the keyboard rather than the mouse, highlight TestMenu with the arrow keys, then press enter, subsequent presses of the up and down arrow keys do nothing. If I then press the left arrow key though, I can then go back to scrolling normally through the Extensions menu. So it’s almost like the focus does get moved to the submenu, but without the submenu being there.

What indicates that this isn’t just a visual bug, though, and that the submenu isn’t just invisible but actually isn’t there at all, is that if I press enter on TestMenu and then enter again, the Hello World command isn’t executed, which it would be normally.

matterhorn103 · January 13, 2025, 12:23am

Ok, I’ve tracked it down to the changes made by this PR which introduced the caching of command script menu paths.

Here’s the git log at the end of the 30th November:

*   666f46f1 (HEAD) Merge pull request #1832 from ghutchis/unbundle-python-scripts
|\  
| * 44c96987 Unbundle all charge and forcefield scripts
|/  
*   c491afc1 Merge pull request #1831 from ghutchis/dna-backbone-support
|\  
| * 2ace4ecb Add support for reading DNA / RNA backbones
| * 1096caba Use PDB to import sequences
* |   141c5953 Merge pull request #1830 from ghutchis/cache-scripts
|\ \  
| * | 6f2dfeca Use a deferred start to initialize force fields
| * | c18cf3a8 Cache command menu paths
| * | c9c992a9 When possible, load script names from cache
=====================================================================================
| |/  
* |   fedca001 Merge pull request #1829 from ghutchis/orca-chelpg
|\ \  
| |/  
|/|   
| * 369a8f26 Parse CHELPG charges from Orca output too
|/  
*   2ab9f8a1 Merge pull request #1820 from weblate/weblate-avogadro-avogadrolibs

I’ve tested the results of building at various points and the bug is introduced at the point where I have drawn the double horizontal line in the log.

So it’s something about the changes in commit c9c992a9. Only scriptloader.cpp was changed significantly there, so that’ll be where we need to look.

ghutchis · January 13, 2025, 12:30am

I’m fine with just reverting that commit if it’s causing problems.

Are you sure it’s c9c992a9 and not c18cf3a8 (or both)? In either case, I’m fine reverting for the release and looking through more carefully later.

matterhorn103 · January 13, 2025, 12:32am

Well I did git checkout <commit> and built from each of those commits, and the resulting binary had the bug in both cases, so I assume that points to the earlier of the two being the culprit.

I looked at what’s saved in the config file and there’s no difference between the Flatpak (which works) and the compiled version (which doesn’t) other than the specific paths used.

matterhorn103 · January 13, 2025, 12:46am

Oh and if I take the current HEAD and revert just c9c992a9, which seems to be possible, the bug is gone

So it’s definitely something from there.

matterhorn103 · January 13, 2025, 12:55am

Specifically, this part:

      } else { // new entry
        scriptList.insert(displayName, filePath);
        // update the cache
        scriptFiles << filePath;
        scriptNames << displayName;
        scriptHashes << QString::number(file.size()) +
                          file.lastModified().toString();
      }

and it seems the problematic part is the QString::number(file.size()) because if I change it to simply

      } else { // new entry
        scriptList.insert(displayName, filePath);
        // update the cache
        scriptFiles << filePath;
        scriptNames << displayName;
        scriptHashes << file.lastModified().toString();
      }

it works.

Am I reading this wrong or are you combining the file size as a string with the last modified timestamp as a string and calling the resulting string a hash without actually hashing it?

ghutchis · January 13, 2025, 1:10am

Anything can be a hash index if it’s likely to be unique. Yes, I included the file size plus the last modified, since that should be unique and hard to fake.

It’s certainly fine to just include the last modified date if that fixes the bug.

ghutchis · January 13, 2025, 1:14am

If you haven’t tried it, next time I suggest Git - git-bisect Documentation - it’s really beautiful. You start with the bad commit (e.g., HEAD) and then indicate a known good commit (e.g. 1.99.0) and it will do the binary search until you find the exact commit that caused the problem.

matterhorn103 · January 13, 2025, 1:30am

Ah thanks, that would have been useful haha. Next time

matterhorn103 · January 13, 2025, 1:49am

Ok, actually the issue seems to be that QDateTime::toString() needs to be passed a format. I guess maybe without the format the second part of the hash returns a non-string value of some kind, because both parts of the hash work on their own, it’s only when concatenated that I get the bug.

Why that should only cause a problem on some specific systems I don’t know. Maybe it’s something to do with me using clang not gcc, no idea.

In any case I’ve fixed it, PR here.

system · January 16, 2025, 1:49am

This topic was automatically closed 3 days after the last reply. New replies are no longer allowed.