Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve wizard ui #2895
Improve wizard ui #2895
Changes from 2 commits
05d2a87
9185956
f55e5b1
e0b7ef1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I'd argue you don't need that class at all, QLabel supports URLs just fine:
https://doc.qt.io/qt-5/qlabel.html#linkActivated
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.
I created this class because of the hover event. I needed to underline the font on hover. I couldn't find an API in QLabel that allows me to underline the label just on hover. There is QLabel::linkHover() but with that, you get just the hover entry signal not the hover out signal. Please correct me if I'm wrong ;)
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.
Honestly I'd consider like it's overdoing it and I'd be fine to have the text underlined. I'd rather spare the extra class with debatable API just for that. :-)
Now if really... one thing which could be done is to use an event filter to catch the enter/leave events and force the style on the label. We're back at having an extra class though, it's just that it'd have a safer API in my views. Indeed it'd be more focused on "change some property of a widget when it is entered, restore it when it's left" so it's easier to have a focused API.
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.
@er-vin
It was wished by the design team ;)
One problem I see with not making an extra class is how to embed the link into the label? I know I can use rich text for that but when rich text is used the font of the label stays not white anymore it uses a different color. Now I know I can work around this by setting the font in the rich text again but it gets then a lot more complicated than it is now. Another possibility would be to create another event filter or even use the HoverEventFilter and rename it to LinkLabelEventFilter but I'm unsure if this brings any benefit to what we have now. What do you think?
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.
Not argument enough in my opinion if that leads to potentially harmful code piling up. ;-)
(not that this single occurrence is super harmful of course, but my experience is that unchallenged design decisions lead to a path of death by 1000 cuts, hence why the question should be at least asked, every single time)
Alright, I prefer the event filter but I see you'll be struggling with that one. Let's scrap that...
Well the main problem really is the QLabel inheritance. I'd be fine even with inheriting from QWidget and have a single QLabel as child (and in a private member). Design wise the problem is that this is still a QLabel with all its API being exposed to the users of LinkLabel. Delegation is way better than inheritance in most cases, here included.
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.
OK... too bad this went in unsolved. Is there any PR I should look forward to regarding this?
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.
@er-vin No not at the moment but I like to do better here. I will create one and let you know then.