-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Gui: overlay dock widgets user interface #7888
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
Conversation
7cf7aac to
9bd4415
Compare
Generating test builds for FreeCAD/FreeCAD#7888
|
Snap test package being generated. Watch this space for updates Edit: build available (until retriggered if new commits are made to this PR) at FreeCAD/FreeCAD-snap#72 (comment) Edit2: updated ☝️ |
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've only looked at the code for the first four commits so far, but it looks very good. I've left a few small comments, nothing blocking a merge if you don't get to them. I love the changes to Parameter, thanks for doing that.
src/Gui/PrefWidgets.cpp
Outdated
| switch(static_cast<int>(getParamType())) { | ||
| case QMetaType::Int: | ||
| case QMetaType::LongLong: | ||
| index = findData((int)getWindowParameter()->GetInt(entryName(), m_Default.toInt())); |
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.
Looks like a couple stray C-style casts are floating around in here.
src/Base/Parameter.cpp
Outdated
| case FCGroup: | ||
| return "FCParamGroup"; | ||
| default: | ||
| return 0; |
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 think some static analyzers are going to flag this: consider nullptr (or throw something).
src/Base/Parameter.cpp
Outdated
| pcTemp)->getAttributes()->getNamedItem( | ||
| XStr("Name").unicodeForm())->getNodeValue()).c_str(); | ||
| // check on filter condition | ||
| if (sFilter == NULL || Name.find(sFilter)!= std::string::npos) { |
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.
NULL -> nullptr
src/Base/Parameter.h
Outdated
|
|
||
| /** @name methods for generic attribute handling */ | ||
| //@{ | ||
| enum ParamType { |
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.
Any reason not to use a class enum?
|
@realthunder I am getting a strange multiply-defined-symbol error when I compile on Windows: Any idea what's causing this? |
9bd4415 to
4de2174
Compare
|
@chennes Commit added for a fix. Please try. I've also made some code changes according to your suggestion. |
Thanks, Windows build compiles now. |
|
Retriggering Snap build Edit: Done. New snap available. |
|
I think the first five commits here are obviously improvements, and can be (uncontroversially) merged. So I propose to cherry-pick those commits, so that the remainder of the review can focus on the more exciting things in this PR. That's:
plus the final new commit,
@realthunder any objection to this? Obviously you'll have to rebase afterwards. |
|
That's great! Thanks @chennes |
|
The cherry-pick is failing because that set of commits won't compile on its own (at least on MSVC): I'm missing a definition for |
|
Please comment out the |
|
OK, those six commits I mentioned above were cherry-picked -- I had to rebase before committing, so the hashes will be different. I also added 848e6cc to comment out those two problematic lines. |
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.
Another round of comments below, focusing entirely on the code in commit 998afb8. I did not complete my review of OverlayWidgets.* due to their size. I'll return to it once some documentation is in place.
src/Gui/ComboView.cpp
Outdated
| if(!prop) | ||
| prop = new PropertyView(this); | ||
|
|
||
| modelIndex = tabs->insertTab(0, splitter,trUtf8("Model")); |
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.
Isn't trUtf8 the Qt4 way of doing it? As of Qt5 everything is UTF-8, right?
src/Gui/ComboView.h
Outdated
| Gui::PropertyView * prop = nullptr; | ||
| Gui::TreePanel * tree = nullptr; | ||
| Gui::TaskView::TaskView * taskPanel = nullptr; |
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.
These are owned by this class, right? Now might be a good opportunity to make them unique_ptr.
src/Gui/CommandView.cpp
Outdated
| sGroup = "View"; | ||
| sMenuText = QT_TR_NOOP("Toggle transparent for all"); | ||
| sToolTipText = QT_TR_NOOP("Toggle transparent for all overlay docked window.\n" | ||
| "This makes the docked widget stay transparent at al times."); |
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.
al -> all
src/Gui/CommandView.cpp
Outdated
| sGroup = "Standard-View"; | ||
| sMenuText = QT_TR_NOOP("Toggle transparent"); | ||
| sToolTipText = QT_TR_NOOP("Toggle transparent mode for the docked widget under cursor.\n" | ||
| "This makes the docked widget stay transparent at al times."); |
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.
al -> all
src/Gui/DockWindowManager.cpp
Outdated
| if (static_cast<QMouseEvent*>(e)->buttons() != Qt::NoButton) | ||
| return false; | ||
| auto pos = QCursor::pos(); | ||
| const int m = 5; |
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.
IMO m is not descriptive enough a name for this variable, especially since it looks like a magic number. Does this behave on high-DPI displays? I'm not a big fan of hardcoding this here.
| drawLine = false; | ||
| update(); | ||
| } | ||
| return hit; |
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.
It doesn't look like client code ever actually uses this return value (which is a bool, despite hit being an int?!).
| // This is necessary to capture any focus lost from switching the tab, | ||
| // otherwise the lost focus will leak to the parent, i.e. MdiArea, which may | ||
| // cause unexpected Mdi sub window switching. | ||
| // setFocusPolicy(Qt::StrongFocus); |
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.
If the comment is true, why is this line commented out?
src/Gui/OverlayWidgets.h
Outdated
| void retranslate(); | ||
| void refreshIcons(); | ||
|
|
||
| enum ReloadMode { |
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.
Probably modernize all of these enums to class enums.
src/Gui/OverlayWidgets.h
Outdated
| void initDockWidget(QDockWidget *, QWidget *); | ||
| void setupDockWidget(QDockWidget *, int dockArea = Qt::NoDockWidgetArea); | ||
| void unsetupDockWidget(QDockWidget *); |
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.
Without any comments it's hard to tell what these actually do -- what is the difference between "init" and "setup"? Overall I think this file, and the individual classes in it, are too long, too complex, and lack documentation. The first two problems we can work on over time, but only if there is documentation to get started.
src/Gui/Tree.cpp
Outdated
| if(ti->type()!=TreeWidget::ObjectType || index.column()>1) | ||
| return 0; | ||
| DocumentObjectItem *item = static_cast<DocumentObjectItem*>(ti); | ||
| App::DocumentObject *obj = item->object()->getObject(); | ||
| auto &prop = index.column()?obj->Label2:obj->Label; |
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.
Looks like mistakenly-included code that overwrites some formatting improvements.
ae2f789 to
367f75f
Compare
367f75f to
ebc6c08
Compare
|
@chennes I've modified and rebase the code according to suggestions. Please check. |
|
I add my vote to merging and then fixing problems one by one in subsequent PRs, even in it's current state I found using this VERY useful |
|
Bump for @chennes if RT is out for now, maybe we can just fix the precompiled in another PR that we merge first and merge this? |
|
I strongly suggest that we merge this asap, if we don't here's whats going to happen :
One tiny typo PR landing on this massive PR (which I'm surprised hasn't happened yet) and we're in for 2 months. And if it happens once, no reason it won't repeat -> rebasing hell and it will never merge. |
Another PR isn't needed. Branch is open to maintainer edit. |
I can't edit a file that is not included in this PR apparently... I'll try to do it locally but not sure I'll be able to push to @realthunder 's repo. EDIT I can't: To github.com:realthunder/FreeCAD.git
! [remote rejected] realthunder-OverlayWidgets -> OverLayWidgets (permission denied)
error: failed to push some refs to 'github.com:realthunder/FreeCAD.git'Ok let's merge this and I'll add the fix right after |
|
Huge congrats to everyone!!! |
I wonder what's the point of removing settings here. Every time a user starts the new version the settings will be removed so that when using an older version all layout settings are completely gone. This is very annoying behaviour! Furthermore I wonder what is the more control you are talking about? The QSettings class was only used to store the size, position, maximized or not, layout of toolbars and dock windows and the visibility of the status bar because (except of the maximized flag and the status bar) these are system-related settings and haven't been added to the user.cfg on purpose. Having all these things in the user.cfg doesn't have any advantage but will sooner or later cause trouble for people who wants to use their user.cfg on different computers. |
|
It may allow for additional control via distributed preference packs perhaps? This would include saving and importing these parameters via the same mechanism I believe. Just my speculation. |
|
Already cleaned and polished the overlay qss a bit #11005 |
I doubt that having junk like this in the user.cfg is of any help: Also, when starting an older version after customizing the layout in the new version then the whole layout will be reverted. Another point is that these data depend on the used Qt version and therefore it makes no sense to share them between different FreeCAD versions that link different Qt versions. These are the reasons why in the past the layout data was written to the registry or FreeCAD.conf file, respectively and not into the user.cfg. |
|
perhaps @realthunder can speak to that and its purpose? |
|
I can't speak about what purpose that serves for this PR but storing all that in user cfg file may make it much easier to get rid of some nasty bugs related to window and toolbar positions by just running a clean config. Some of these bugs could even prevent freecad from starting on windows for some people. |
|
It must be due to this: #11012 - I'll check what is wrong and provide fix |
|
It's this line: |
|
Can you provide the fix? I won't be able to do that in 12 hours or so |
|
Some features do not automatically open the task dialog when set to Auto show on edit #11637 |
…|320px]] | добавлена функция, позволяющая накладывать прикрепляемые виджеты (дерево и прозрачность задач). [FreeCAD/FreeCAD#7888 Pull request #7888] |}"
…|320px]] | добавлена функция, позволяющая накладывать прикрепляемые виджеты (дерево и прозрачность задач). [FreeCAD/FreeCAD#7888 Pull request #7888] |}"


Closes #7756
Forum thread https://forum.freecadweb.org/viewtopic.php?f=34&t=73280