GrandOrgue / grandorgue

GrandOrgue software
Other
150 stars 39 forks source link

Fixed error messages after multiple runs of GrandOrgue ftom Appimage with a demo organ https://github.com/GrandOrgue/grandorgue/issues/1660 #1681

Closed oleg68 closed 8 months ago

oleg68 commented 8 months ago

This is the first PR related to #1660

Because running GrandOrgue from applimage causing unpacking it to a temporary directory, the demo Organ may be registered multiple time with different paths. Old paths are not more valid, because the old directories have already been deleted.

Earlier opening an organ from a package were only by ArchiveId and the path was not taken in account. It caused trying to open the archive from all paths had been registered and logging error messages on each invalid path.

This PR adds passing the archive path to GOArchiveManager::LoadArchive that makes unnecessary looking over invalid paths. So this PR eliminates logging extra error messages.

Multiple demo organs registered are still an issue and will be fixed with another PR.

larspalo commented 8 months ago

@oleg68 Would it be an option to just remove the demo sample set from the AppImage build?

larspalo commented 8 months ago

@oleg68 Actually, earlier we had a separate build artifact for only the demo sample set to download. Perhaps it would be a good idea to remove the default inclusion of the demo sample set from all builds if not explicitly set to be included and have a separate target for only the demo sample set (or even just include a link to it as it doesn't change that often and could be just downloaded once...).

What this would do is to reduce the sheer amounts of different demo sample sets some of us have floating around when we're testing different builds... Installation of the demo sample set would be a choice and exist only in one instance (if not the user decides otherwise when building from source).

oleg68 commented 8 months ago

@larspalo I object to remove the demo from appimage because i is intended for using by not advanced users for whom it is difficult to download a separate sample set and to open it with GrandOrgue.

The next PR will automatically remove unexisting organs that were opened from a temporary directory. So unpacking GO under the temporary directory will solve your problem of testing multiple versions.

larspalo commented 8 months ago

So unpacking GO under the temporary directory will solve your problem of testing multiple versions.

No, it won't. Every time I download an archive of GO to test a PR a new demo sample set is registered with it.

I object to remove the demo from appimage because i is intended for using by not advanced users for whom it is difficult to download a separate sample set and to open it with GrandOrgue.

One doesn't "Open" a package in GO, it's "Loaded". If it's too difficult to download and place the .orgue package in the package folder for GO and then start GO - the intelligence of people in the world is really going downhill...

larspalo commented 8 months ago

@oleg68 Just to prove my point, here is a screenshot of the organ list from GrandOrgue on my computer.

Skärmbild från 2023-10-14 17-33-00

The fact that most of the demo sample sets listed are actually deleted but GO doesn't report them as missing is a bug in itself...

I usually clean up to list from time to time, but it's quite un-necessary that we include the demo sample set in any build that is not in fact a release.

oleg68 commented 8 months ago

The fact that most of the demo sample sets listed are actually deleted but GO doesn't report them as missing is a bug in itself...

I hope this bug is fixed with #1682. Earlier the existance was checked by ArchiveId rather than by path, so GO considered an absent archive as existing because there was another existing archive with the same id.

oleg68 commented 8 months ago

So unpacking GO under the temporary directory will solve your problem of testing multiple versions.

No, it won't. Every time I download an archive of GO to test a PR a new demo sample set is registered with it.

1682 will help you if you unpack GO under the temp dir.

I object to remove the demo from appimage because i is intended for using by not advanced users for whom it is difficult to download a separate sample set and to open it with GrandOrgue.

One doesn't "Open" a package in GO, it's "Loaded". If it's too difficult to download and place the .orgue package in the package folder for GO and then start GO - the intelligence of people in the world is really going downhill...

The package folder does not exist before GO runs for the first time. The current behaviour is the most simple: to download appimage, to set it executable, to double click and to play without any extra steps. Necessarity of downloading a sample set would add more complexity. So I'd leave the demo organ packaged in appimage.

larspalo commented 8 months ago

So I'd leave the demo organ packaged in appimage.

Ok, but it should be removed from all others, and instead be made available as an optional download.

larspalo commented 8 months ago

https://github.com/GrandOrgue/grandorgue/pull/1682 will help you if you unpack GO under the temp dir.

A true solution must work no matter what directory is used. Each version of an archive will be extracted to another base directory by default. An executable, archive (.tar.gz, .zip or whatever really) should not be required to be extracted to a specific place, it is the .orgue package that should be placed in a specific place...

oleg68 commented 8 months ago

So I'd leave the demo organ packaged in appimage.

Ok, but it should be removed from all others, and instead be made available as an optional download.

I ageree with removing the demo from .zip and .tar.gz, because they usually used for resting where GO has already installed.

But I still insist on presenting the demoin all installation packages, because they are overwritten by the next installation and no multiplication occurs.

But changing shipping the demo is a subject of a separate issue. @larspalo do you have any reasons why not to get rid of error messages when missing archives exist, but the current organ is loaded from an existing one?

oleg68 commented 8 months ago

@larspalo

There are lots of places in GO where organ packages are referenced by archive id instead of by path. This PR eliminates one of them - loading an organ, #1682 fixes another place - checking the organ for existence.

But, of cause, a dependency between organ packages are still by id rather than by full path and now I don't have any ideas how to improve it. Lets continue the discussion in the discussion area, not here.