-
Notifications
You must be signed in to change notification settings - Fork 3.2k
mac/menu: pre-fill mpv and system information when creating an issue #17211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
preparation for more version information and pre-filling the issue template.
adding IDs to the fields makes them pre-fillable with a URL query.
|
|
||
| const char *app_bridge_pl_version(void) | ||
| { | ||
| return PL_VERSION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe i can use pl_version() directly and remove that wrapper/indirection? https://github.com/haasn/libplacebo/blob/bc90ef94944a3dcaab324b86d3e3769ad1d8698b/src/include/libplacebo/config.h.in#L32
when opening a new issue from the menu bar when can easily pre-fill the mpv and system information since they are easily available in our code.
782dbee to
a6042bc
Compare
|
Why is this macos specific? |
|
you need to be a bit more specific which part you mean? the version stuff is just a necessity for the menu adjustments. the former could probably be replaced with something none-system specific (c structure or something). the latter is macOS only anyway and just one possibility how to use it. |
We have menu on all platforms, why not add it there? |
|
I think this would be better suited for the context menu than the current menu which has only a few important items since 99.999% of users will never use it. It still makes sense to update the macOS menu since it already had a "Report Issue" item. |
I disagree, context menu should be clean and used only for immediate player control. Command menu is for things like "open docs", "edit config', "report issue", which are all the same class of helper commands that 99.999% of users never uses. |
|
|
Out of scope of what? You are adding swift code for checking ffmpeg library version, which is both not necessary and bloated. Furthermore such code can work on any platform.
Yes, and I asked why you are adding macos specific menu entry for feature that can be available on any platform. It's platform agnostic. |
i asked you to be specific about what your issues is and you replied to the question with:
i replied to that and this is imo clearly out of scope. you are asking me to add a 'feature' to something else because i added a similar unrelated feature to a platform specific part of the code.
the menu entry is it's already there and i am just changing the URL, it wasn't added here. it's not completely platform agnostic because:
i suggested early that can possibly be changed. i know i am going to be very pedantic here, it's not >checking< any versions (eg it's not a version check like here), the code is just just >reading< versions data (like similar to our version output like mentioned above) for outputting it. before i write/argue any more and fishing a bit in the dark (also possibly making a dumbass out of myself), please let's figure out what the actual issue is and what should be changed.
is the issue just point 2. and how the mpv/library version data is retrieved or do you have something else on mind? sorry if i misunderstood your intention, just trying to figure out what should be changed. |
Changing URL involves 100 lines of support code, that produces this URL. It's not "just" changing the URL.
Yes, I'm asking to not build more walled macos specific code that is not usable on other platforms. One thing is supporting platform specific API, the other is building features that are locked to single platform without good reason to do so.
Then design it as any other platform specific code in osdep. Imagine now that someone wants to implement this for other platform, they would need to duplicate all the logic from macos code or remove it all together. I'm not asking you to implement this for other platforms, but I think we should be forward thinking about maintainability of the code. |
|
sorry if it looks like it but i am not trying to be hostile here. just trying to figure out what exactly you want. first of all you are repeating yourself, you are over exaggerating and i didn't even dispute that/those points, as mentioned in my posts before. sorry @kasper93 you are very unspecific again (which is the problem atm), you are not specifying what exactly you consider a 'feature' here, what problem you want me to solve exactly and avoiding my question at the bottom.
that exact point "reads the platform version" is indisputable. for that i need to add code to the macOS side anyway, to add this specific part to the macOS menu. retrieving that info outside of platform specific code would just reuse this code and basically just wrap it, since it's needed at two separate place then. though it also depends on the above. could we please concentrate on the exact things that should be implemented and how it should look like, since there are various different ways and possibilities? i can't work with a general "this should be there like anywhere else" without knowing what exactly >this< this or what is on your mind. |
I'm repeating myself, because I'm not sure which part is hard to understand. I'm vague, because I don't want to tell you what to do and how to do it. I'm telling you what not to do.
url generation is trivial and can be duplicated if it makes it easier. However ideally it would be in
Yes, I asked at the beginning,
Other information for other platforms is outside of the scope of this PR. And it would be platform specific, obviously. In case of macos it's single line of code as shown by this PR.
It can be Or if you agree to move this menu item to select.lua, it can be even implemented in lua, as native code doesn't really need this information, except single place. I really don't care, all I ask is to not build upon the macos swift code. I don't ask you to implement it for any other platform, but make it possible to implement it.
I doubt they all need to be individual, they can be bundled in single string same as
I don't want anything. I'm only voicing my concern that prefiling mpv information may be useful not only for macos users. I think swift layer should be minimal and not expanded to handle tasks that are not required to be used there. Let me know if you disagree. There are three modules here "menu", "mpv info", "system info" and only the last "system" should live in swift. I understand that this menu item is not new, but existing duplication is not excuse the build and move more code to swift. If we want to write mpv in safe language, we should move to rust, not swift. |
this adds IDs to all textarea fields in our issue templates. this adds the ability to pre-fill those fields via URL queries.
as an example, i added the pre-filling to the menu to open an issue and macOS. an example URL on my fork https://github.com/Akemi/mpv/issues/new?template=2_bug_report_macos.yml&mpv_info=mpv%20v0.41.0-49-ge3ebeb27e-dirty%20Copyright%20©%202000-2025%20mpv%2FMPlayer%2Fmplayer2%20projects%0A%20built%20on%20Dec%2031%202025%2015%3A25%3A16%0Alibplacebo%20version%3A%20v7.357.0%20(v7.351.0-87-g9bffcaf2)%0AFFmpeg%20version%3A%208.0.1%0AFFmpeg%20library%20versions%3A%0A%20%20%20libavcodec%20%20%20%20%20%2062.11.100%0A%20%20%20libavdevice%20%20%20%20%2062.1.100%0A%20%20%20libavfilter%20%20%20%20%2011.4.100%0A%20%20%20libavformat%20%20%20%20%2062.3.100%0A%20%20%20libavutil%20%20%20%20%20%20%2060.8.100%0A%20%20%20libswresample%20%20%206.1.100%0A%20%20%20libswscale%20%20%20%20%20%209.1.100&other_info=-%20macOS%20version%3A%2015.7.2%20(Build%2024G325)%0A-%20Source%20of%20mpv%3A%0A-%20Latest%20known%20working%20version%3A%0A-%20Issue%20started%20after%20the%20following%20happened%3A
this kinda replicates the version output we already have in our logs +
--version.kinda related to #13968.