Closed OPNA2608 closed 4 years ago
Hey, thanks for pointing this out! I actually hadn't paid any attention to those lines before.
Poking around a bit it seems like they were added by qtcreator by default, which is a bit surprising because I agree it looks nonstandard. I'll take a look and fix it up.
Okay, I think this should be good! Thanks for the helpful examples too. Let me know if there are any other problems.
Hope you don't mind if I escalate this issue abit and make it about various small install-time things I'm noticing while packaging ptcollab for a Linux/macOS package manager, making an issue for each of them feels abit excessive.
I commented something on your commit that needs to be fixed for the application icon to be installed properly. If you don't plan to add other fixed resolutions of that icon though, then you can simplify it abit.
Where's the icon from, is there a higher-quality source? The freedesktop's icon theme specifications permit multiple nominal resolutions for better-quality bitmap icons at higher resolutions and optional SVG for smooth scalability, hence the resolution-looping code in BambooTracker. (an SVG made no sense for us because the original icon is pixel art)
If you have an SVG source (or I can trace your bitmap icon) then you / I could add that to the install process - the target path for installing would be $$PREFIX/share/icons/scalable/apps/ptcollab.svg
I saw that the ZIPs on the release page have sample instruments and songs, however in the repository I can only find the zipped-up instruments - that also don't have install targets. You should definitely add all of those to the repo (preferably in unzipped form to cut down on dependencies) and install them to $$PREFIX/share/ptcollab/…
. For the instrument taken straight from pxtone, creating a pxtone
subdirectory to cleanly separate them from (maybe in the future?) user-submitted instruments and songs would seem like the sensible approach to me.
Lastly, the version information is currently broken for me.
This is due to the project file extrapolating the version information via git
& the .git
repository directory at build time.
For reproducibility reasons, the build manager I'm using throws away the .git
directory before starting the build. Build inputs, including the source code, are verified with a hash to ensure as much reproducibility as possible, and the .git
directory is sadly a source of indeterminism due to various actions being able to change its contents despite fetching a pinned version of the code. Would it be possible to replace this mechanism with a simple header file that contains the current release version, however finely-grained you need it to be? Our example from BambooTracker here.
Ah, got it. Thanks so much for this! Here I was thinking using the git version was clever, haha.
There is an icon.svg in the src directory - it's unused in the code but was what generated the mac icons. I added the sample songs to the zip after realizing that mistake too. It's still in a zip, but I'll get to fixing that and the other issues sometimes today (unless you'd be willing to take a stab?).
Ah damn, I had already spent some time nicely recreating the icon in Inkscape. :smile:
(unless you'd be willing to take a stab?)
I'm abit busy with tasks, but I'll see what I can do. Could take awhile though, so you're prolly faster.
Haha, your version may well be crisper than the existing icon. And that sounds good, don't feel any pressure - thanks again!
All right, this should be done! Assuming I didn't misconfigure anything else...
The 16px icon seems too small (has transparent space on some sides instead of fitting to the border) & off-center.
Aside from this, it builds, installs and works fine (offline at least, haven't tried any online kerfuffle yet). Thanks!
Haha, your version may well be crisper than the existing icon.
Well it's a vector graphic, it can't get any crisper :stuck_out_tongue_winking_eye:
Ah, great! Glad all of that works, thanks again!
As for the small icon, it's borrowed from an older project as a favicon and was set to 15x15 so the triangle could be centred in the dark box and not have to stretch. It didn't seem distracting in the browser to me, but maybe it's distracting in another desktop view? (or maybe it's always been distracting and I've just gotten numb to it haha)
Sorry for the late response.
The smaller icon is not a deal breaker, but I can never unsee it now xp. It just looks unintentional I guess.
I tested it online and everything worked as well, very nice. One of my friends on macOS was bummed because she's on 10.13 High Sierra but the build manually sets the lowest supported version to 10.14 Mojave. Out of curiosity, is there something that needs this bumped target to fix? The default for Qt 5.15 is 10.13.
https://github.com/yuxshao/ptcollab/blob/4827b3e895909f02980140efd30652afe257da8e/src/editor.pro#L10
I consider the issue solved in any case, many thanks!
Great, glad it worked for the most part! Thanks again for helping with this.
I'll revisit why it's 10.14, but my vague memory is that when it wasn't set, there a number of errors for C++17 features. But I will double check and try to fix that.
Edit: Yeah, unfortunately it has to do with the fact that std::variant and std::optional (which are used pretty heavily in the source) are not available 10.14 or earlier :(
This applies to Linux in particular, but the other major OS' would benefit from it too: Please make the installation prefix user-defined, hard-coding them is never a good idea.
https://github.com/yuxshao/ptcollab/blob/cb670e653e707c936e5bcc862c8bfb392362fe20/src/editor.pro#L177-L180
The user / their build system manager might want to install them somewhere else, mine for example expects the project file to handle the quasi-standard
PREFIX
to designate what should be considered the root directory to place/bin
,/lib
,/share/[projectname]
etc under. This path can be very different from/opt
; in my case,PREFIX=/nix/store/ccbbas68vlr0d52h8wda9l30rma9gzn8-ptcollab-0.3.4
will be passed to themake
invocation. A default should of course still be supplied, which can be/opt
or/usr
.For an example how to handle this nicely, you can check out BambooTracker's project file.
https://github.com/rerrahkr/BambooTracker/blob/9fd422431e99f6bd76ad79db7fe1b0e67cf54430/BambooTracker/BambooTracker.pro#L25-L39
https://github.com/rerrahkr/BambooTracker/blob/9fd422431e99f6bd76ad79db7fe1b0e67cf54430/BambooTracker/BambooTracker.pro#L602-L627