Skip to content
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

OS Info Improvements #2618

Merged
merged 2 commits into from Oct 25, 2020
Merged

OS Info Improvements #2618

merged 2 commits into from Oct 25, 2020

Conversation

codebrainz
Copy link
Member

@codebrainz codebrainz commented Oct 10, 2020

Handles macOS and older GLibs better, as a follow-up for #2498.

Some tests so far:

  • Apple macOS
    • OS: macOS
  • Win7
    • OS: Windows 7 SP17Vista SP2Vista SP1VistaXP SP3XP SP2XP SP1XP (Unknown)
  • Win10 w/ Msys2 (same output for 32 and 64-bit msys2):
    • OS: Windows 10 2004 (Unknown)
  • Debian
    • OS: Debian GNU/Linux bullseye/sid
  • Ubuntu 20.04
    • OS: Ubuntu 20.04.1 LTS (focal)
    • OS: Linux Mint 20 (ulyana)
  • Ubuntu 20.04 with incompatible GLib
    • OS: Linux

If it gets enough testing, could make 1.37, else we can leave for next release.

@elextr
Copy link
Member

elextr commented Oct 11, 2020

WFM, result in list above.

@frlan
Copy link
Member

frlan commented Oct 11, 2020

Debian:

OS: Debian GNU/Linux bullseye/sid

@eht16
Copy link
Member

eht16 commented Oct 11, 2020

Windows 7:
OS: Windows 7 SP17Vista SP2Vista SP1VistaXP SP3XP SP2XP SP1XP (Unknown)

src/utils.c Outdated Show resolved Hide resolved
@codebrainz
Copy link
Member Author

Debian:

OS: Debian GNU/Linux bullseye/sid

Is the "bullseye/sid" part parenthesized or exactly as above?

@frlan
Copy link
Member

frlan commented Oct 11, 2020

As in the quote
image

@elextr
Copy link
Member

elextr commented Oct 11, 2020

Well with the weird Debian naming scheme I guess it doesn't have a per version nickname like fossa for Ubuntu, so having no nickname makes (twisted Debian) sense. Anyway it provides information on the OS which is the main thing.

Copy link
Member

@elextr elextr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also making the strings untranslated makes it mergable now we are in string freeze :)

src/utils.c Show resolved Hide resolved
@eht16
Copy link
Member

eht16 commented Oct 13, 2020

Just for completeness:
OS: Arch Linux

@techee
Copy link
Member

techee commented Oct 15, 2020

I get

16:13:53: Geany INFO		: OS: macOS

Question: the function is exported as an API function to plugins. Is this something plugins are actually interested in?

@frlan
Copy link
Member

frlan commented Oct 15, 2020

@elextr Bullseye is actually name of the next major one -- Running testing here

@codebrainz Just compiled your branch with nix package manager -- still describing itself as Debian:

Geany-INFO: 20:16:45.424: Geany 1.37, C
Geany-INFO: 20:16:45.424: GTK 3.24.21, GLib 2.64.5
Geany-INFO: 20:16:45.426: OS: Debian GNU/Linux bullseye/sid
Geany-INFO: 20:16:45.426: System data dir: /nix/store/cyk52jflc1v4aj5pqry8v57sv9p1ha9h-geany-1.37git/share/geany
…

But I guess this is about g_get_os_inf() ist working.

@elextr
Copy link
Member

elextr commented Oct 15, 2020

@techee AFAICT its not actually in the API, the documentation message is not a doxygen comment and the function isn't marked GEANY_API_SYMBOL.

In theory a plugin might use the info, but given the variability (and as @frlan showed the fact that derived distros may show as the base distro) its probably not much use to a plugin.

@elextr
Copy link
Member

elextr commented Oct 15, 2020

@frlan the g_get_os_info() docs say "On Linux this comes from the /etc/os-release file." so if your distro is derived from Debian its probably not modifying that file. So as much as I like to blame G* for everything, in this case its probably not their fault.

@b4n
Copy link
Member

b4n commented Oct 15, 2020

@techee AFAICT its not actually in the API, the documentation message is not a doxygen comment and the function isn't marked GEANY_API_SYMBOL.

No, but it has an API version in the @since tag, which doesn't make much sense.

In theory a plugin might use the info, but given the variability (and as @frlan showed the fact that derived distros may show as the base distro) its probably not much use to a plugin.

Also, I don't see why a plugin would need that info if it's not an actual mean of identifying the platform (and even then, it's probably not a good metric to do anything). As is it's really meant as a human-readable clue rather than something formal. So ATM I don't see any reason for it to make it to plugin API anytime soon.

@codebrainz
Copy link
Member Author

codebrainz commented Oct 15, 2020

No, but it has an API version in the @since tag, which doesn't make much sense.

I just put that so it was easy to see when it was added, but agree the (API 239) part doesn't make a whole lot of sense.

@elextr
Copy link
Member

elextr commented Oct 16, 2020

@SInCE sorry @codebrainz didn't mean to ping you, he just forgot to quote some Doxygen syntax.

@codebrainz
Copy link
Member Author

Updated according the comments, squashed, rebased, and force pushed.

@codebrainz codebrainz added this to the 1.37 milestone Oct 17, 2020
Copy link
Member

@elextr elextr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGBI now

@codebrainz
Copy link
Member Author

Should we merge this last minute, or wait till next release? I don't mind either way, it's not like it's a critical enhancement.

@eht16
Copy link
Member

eht16 commented Oct 25, 2020

I'd like to have it merged. It will help users to help us when handling bug reports and the change is pretty harmless.

@b4n b4n merged commit ad54ee5 into geany:master Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants