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

Organized version information #3781

Merged
merged 17 commits into from
Jun 4, 2022
Merged

Conversation

zneix
Copy link
Collaborator

@zneix zneix commented May 29, 2022

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Organized version information shown in About page:

  • moved Qt version information to the top (main motivation)
  • organized and split compile-time and runtime information since it was getting a bit cluttered in there; diff there is stupid, just read new code
  • removed ancient irrelevant comments
  • added new bool isModified() member to Version class - used to determine whether the vcs tree was compiled or not at the time of building the app. Idea adopted from Golang's source code:

Added information about running in DEBUG mode to fullVersion:

At first, I wanted to only add this to AboutPage.cpp but I figured it might be good to be consistent in other places (result of --version and/or title window); however if this change is "too invasive" I will revert it and add this information to AboutPage.cpp only.

Previews:

built in debug mode with modified tree + no nightly:
debug modified no nightly

built in non-debug (release) mode with clean tree + nightly:
release clean nightly

zneix added 2 commits May 30, 2022 00:37
- moved Qt version information to the top (main motivation)
- organized and split compile-time and runtime information since it was getting a bit cluttered in there
- removed old unused comments
- added new `GIT_MODIFIED` variable - used to determine whether the vcs tree was compiled or not at the time of building the app
I can imagine that in some cases it might be very helpful to determine whether one is running a DEBUG build (e.g. in the process of troubleshooting/determining crash causes).
add a qmake variant as well since it involves basically no hassle
@zneix zneix force-pushed the zneix/chore/organize-version-information branch from a2a8532 to e383c89 Compare May 29, 2022 23:50
@kornes
Copy link
Contributor

kornes commented May 30, 2022

while on about page topic, maybe we could move contributors list to chatterino api (since it needs to be curated and we cant use github directly)? so there wont be need to bundle 50 avatars and txt file.

@zneix
Copy link
Collaborator Author

zneix commented May 30, 2022

while on about page topic, maybe we could move contributors list to chatterino api (since it needs to be curated and we cant use github directly)? so there wont be need to bundle 50 avatars and txt file.

Firstly, I'm focusing on version-related changes here (not just in About page - it's not about reworking that page but reworking logic around version information). So even if I'd agree, that'd be something for another PR.
Secondly, No. There is a good reason we include authors' information locally, but I won't go over that again here since this was already raised in #2139 (comment)

Besides, I don't think it's a lot of sacrifice to keep that data locally.

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Looks good! At some point we should just make two functions, one that generates "build information" and one that generates "runtime information" so diffs looks nicer in the future

src/common/Version.hpp Show resolved Hide resolved
Copy link
Collaborator

@Felanbird Felanbird left a comment

Choose a reason for hiding this comment

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

The new wrapped around line of information cannot be copied.

2022-05-30_08-59-37

Otherwise good change, I'd literally take any change to get that info wrapped around, since I'm sick of seeing the bottom scrollbar.

@zneix
Copy link
Collaborator Author

zneix commented May 30, 2022

The new wrapped around line of information cannot be copied.

Thanks for catching this!

Before my changes, the structure was something like:
parent layout -> QGroupBox -> QLabel with version
But after I wanted to add a line break in between I turned it to something like:
parent layout -> QGroupBox -> QVBoxLayout -> (QLabel , QLabel)
Even though QVBoxLayout renders its labels vertically, they're in fact completely "isolated" so that it's impossible to select text from multiple QLabel's at once, even with right mouse interaction flags being set.

I've tried to find a reasonable solution to this and getting rid of second QLabel + putting a simple <br> between lines seems to be the only non-overengineered quick solution.
One thing I'm not so sure about this is that with only one QLabel, the QVBoxLayout is technically useless, however I've noticed that its presence makes some nice "padding" that pther QGroupBox'es in About page use, which makes Version's QGroupBox a bit more inline with the other ones (they all have a QVBoxLayout holding all the elements), look at the padding:
ocd satisfied
Like I said, even though QVBoxLayout is not necessary, the lack of it makes things look a bit more 'ugly':
reeeee

So I'm thinking I'll just keep the QVBoxLayout here for the sake of being consistent with the paddings, even if it's going to hold only 1 element. Such a minor thing and so much writeup... but alas I've chosen the modification visible on the 1st screenshot in 4a60365.

zneix added 2 commits May 30, 2022 16:28
also make it impossible to override its value
@zneix zneix force-pushed the zneix/chore/organize-version-information branch from 4b648e1 to 398f81e Compare May 30, 2022 15:28
@zneix
Copy link
Collaborator Author

zneix commented May 30, 2022

I decided it'll be cleaner to determine the boolean at qmake/cmake step already, which should also take care of qmake compilation issues. While doing this I've decided it should be impossible to override the value of GIT_MODIFIED since then it'd be pointless to have this "variable" in the first place. And there's no need for any fallback through env variables since if git command fails, Version::isModified() would be set to false by default.

@zneix zneix requested a review from pajlada May 30, 2022 15:32
We don't care about this variable's value, but rather the +='d DEFINE - although idk how to show whether it was set or not in this message() function, so I don't care about it - qmake is deprecated already anyway
Copy link
Collaborator

@Mm2PL Mm2PL left a comment

Choose a reason for hiding this comment

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

nice PR, just small things

chatterino.pro Outdated Show resolved Hide resolved
cmake/GIT.cmake Outdated Show resolved Hide resolved
zneix and others added 3 commits June 1, 2022 13:40
Also removed GIT_HASH since GIT_COMMIT already gives us its value
They can be accessed with the `buildString()` and `runningString()` methods

The String building now uses string appending rather than QString args
where it makes sense to make the code more readable.
This might be a small performance loss, but since the code only runs
once readability > performance.
@pajlada
Copy link
Member

pajlada commented Jun 4, 2022

image
I removed a ; before the built part
I uppercased the running string

Both strings are now independently generated once on startup by the Version class, and returned with buildString() and runningString()

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

I'm happy with the code as it looks now, feel free to take a polishing step on it if you'd like, or come with feedback!

@zneix zneix enabled auto-merge (squash) June 4, 2022 18:40
@zneix zneix merged commit a7939b7 into master Jun 4, 2022
@zneix zneix deleted the zneix/chore/organize-version-information branch June 4, 2022 19:00
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

5 participants