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

[Sketcher] Make constraint label smaller and styleable #5091

Merged
merged 10 commits into from Oct 13, 2021

Conversation

chennes
Copy link
Member

@chennes chennes commented Oct 6, 2021

As discussed in the FreeCAD forums, this PR exposes the Sketcher Constraint Status Label to user-configurable styling via both preferences (no GUI, just settable in user.cfg) and via the Qt stylesheet. This is accomplished by creating a new widget called StatefulLabel. This widget has a Q_PROPERTY called "state" which can be dynamically altered at runtime. When the state is changed, the user preferences are optionally scanned for an entry as specified by the label creation code, then the stylesheet is checked, and finally, the default values are used as a fallback.

This PR also significantly reduces the verbosity of the message, eliminates the possibility of multiple overlapping messages (e.g. "redundant" and also "partially redundant"), and moves the clickable portion of the label to its own UrlLabel (which can be styled, unlike an embedded <a> element).

chennes and others added 5 commits October 2, 2021 22:15
The new StatefulLabel widget is designed to be customizable by optional
preferences entries, Qt stylesheets, and default styles, set on a
per-state basis, where "state" is a Qt property that can be changed
dynamically at runtime.
Reduce the size and verbosity of the solver and constraint status
messages, and make them styleable both via stylesheets and user
preferences.
With Sketcher's constraint status label now exposed to stylesheet
control on a per-state basis, add basic styling that is more visible in
this dark stylesheet.
@chennes chennes added ✏️ Sketcher Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD labels Oct 6, 2021
@freecadci
Copy link

pipeline status for feature branch PR_5091. Pipeline #383250984 was triggered at 4154bdd. All CI branch pipelines.

@freecadci
Copy link

pipeline status for feature branch PR_5091. Pipeline #383771254 was triggered at 6858d93. All CI branch pipelines.

@freecadci
Copy link

pipeline status for feature branch PR_5091. Pipeline #383431392 was triggered at 4154bdd. All CI branch pipelines.

@freecadci
Copy link

pipeline status for feature branch PR_5091. Pipeline #383839759 was triggered at 6858d93. All CI branch pipelines.

@chennes chennes marked this pull request as ready for review October 7, 2021 13:28
@abdullahtahiriyo
Copy link
Contributor

@chennes

It is conflicted, could you please resolve the conflict?

@chennes
Copy link
Member Author

chennes commented Oct 11, 2021

Yes, I caused the conflict 😄 -- I'll take care of it.

@freecadci
Copy link

pipeline status for feature branch PR_5091. Pipeline #386440259 was triggered at 6bcda71. All CI branch pipelines.

@abdullahtahiriyo
Copy link
Contributor

CI is complaining about an unused parameter:

../../../src/Gui/Widgets.cpp: In member function 'virtual void Gui::StatefulLabel::OnChange(Base::Subject<const char*>&, const char*)':
../../../src/Gui/Widgets.cpp:935:58: warning: unused parameter 'rCaller' [-Wunused-parameter]
  935 | void StatefulLabel::OnChange(Base::Subject<const char*>& rCaller, const char* rcReason)
      |                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
make[2]: *** [src/Gui/CMakeFiles/FreeCADGui.dir/build.make:4658: src/Gui/CMakeFiles/FreeCADGui.dir/Widgets.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:6872: src/Gui/CMakeFiles/FreeCADGui.dir/all] Error 2
make: *** [Makefile:130: all] Error 2
Cleaning up project directory and file based variables
ERROR: Job failed: exit code 1

@berndhahnebach
Copy link
Contributor

berndhahnebach commented Oct 12, 2021

Just restared the job and it really fails on 6bcda71

@abdullahtahiriyo
Copy link
Contributor

@chennes
No rush. Just making sure you get notified.

@chennes
Copy link
Member Author

chennes commented Oct 12, 2021

Thanks, @abdullahtahiriyo -- I think I have resolved the issue (this time I tested on Linux!). I've also added a commit to include basic styling of the label in all of our included stylesheets. I used the coloring you originally had in the code, except to switch black text to white on the dark styles, and to make the URL a lighter color on the dark styles. I figure folks like @turn211 will want to go back in and tweak those colors for better visibility, but this is a good starting place.

@abdullahtahiriyo
Copy link
Contributor

@berndhahnebach
Do you know if we need to retrigger the gitlab pipeline somehow after the new commits?

If yes, if it is something we can do (without having to disturb you), please let us know how to do it. :)

@freecadci
Copy link

pipeline status for feature branch PR_5091. Pipeline #387344400 was triggered at dc3583e. All CI branch pipelines.

@abdullahtahiriyo
Copy link
Contributor

@chennes
For reasons I cannot understand the button "Rebase and Merge" is present but cannot be clicked. I tried login out and back in in GitHub.

I have clicked "update branch", although this is something that was never needed to do a "rebase and merge". This has triggered a new gitlab pipeline check.

I will try later on during the day. If I can not, I suggest you merge it, as I have reviewed the changes and tested it and I am satisfied.

Copy link
Contributor

@abdullahtahiriyo abdullahtahiriyo left a comment

Choose a reason for hiding this comment

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

I am trying to unveil the reason why I cannot merge this PR. So I requested another review from myself, which I am approving now.

@abdullahtahiriyo
Copy link
Contributor

@chennes

Very odd. I can rebase and merge my own PR:
#5109

which has not passed the tests (this is something we could do before, i.e. merge without passing the tests)

But I could not rebase and merge this one, when it had passed the tests (before hitting update)...

@abdullahtahiriyo
Copy link
Contributor

@berndhahnebach

Sorry to disturb you again. I cannot make CI run the last commit in this thread.

In the GitLab UI I can re-trigger previous pipelines...

@abdullahtahiriyo abdullahtahiriyo merged commit e38d06a into FreeCAD:master Oct 13, 2021
@abdullahtahiriyo
Copy link
Contributor

At the end, the issue appears to be that the PR could not be "rebased and merged" (which is what I usually do to have cleaner history). I have now "created a merge commit". Thanks for your support.

@luzpaz luzpaz added WB Sketcher Related to the Sketcher Workbench and removed ✏️ Sketcher labels Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD WB Sketcher Related to the Sketcher Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants