-
Notifications
You must be signed in to change notification settings - Fork 2
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
Cad floater #59
base: master
Are you sure you want to change the base?
Cad floater #59
Conversation
</property> | ||
<property name="font"> | ||
<font> | ||
<pointsize>8</pointsize> |
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.
The distance is the only lineedit with a fixed size, which results in a smaller font size here (hidpi screen) than the other floaters.
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
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.
Has that been changed?
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.
Yes in d65e63f
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 8 very small (smaller than the rest of the fonts)? I'm a bit afraid for visually impaired users.
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.
Here (non-hidpi screen) it's actually the default it seems ? I'll remove the setting so it would display at the same size than other UI elements on all setups, and adapt with font size setting (in QGIS settings).
|
||
/** | ||
* move the widget | ||
* \param pos position of the cursor |
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.
Watch out for asterisk indentation
/**
* Text
* \note ...
*/
Also, it's often nice to just specify parameters inline Move the widget to the cursor \a position.
.
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
|
||
private slots: | ||
|
||
void changeX( QString text ); |
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.
QString
as parameter should (almost?) always be const QString &text
.
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, I guess this also applies to updatePos( QPoint pos )
?
* | ||
* \param active | ||
*/ | ||
void setActive( bool active ); |
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 that's never used as a slot (only directly called). Just make it a public method
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'll refactor this a little bit (remove the toggleFloater function, and connect the action directly to floater::setActive). I think it's a bit cleaner (e.g. the settings to store the floater's visibility are now read/written in the floater class).
|
||
/** | ||
* Whether the floater is active or not. | ||
* Note that the floater may be active but not visible (e.g. if the mouse is not over the canvas). |
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.
Add \since QGIS 3.8
to all new public methods (on [GUI|CORE|ANALYSIS]_EXPORT
classes
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
Hi @olivierdalang I opened a PR here for an off the record pre-review. It's looking really good, I like it a lot! There seem to be a couple of smaller issues (font-size of distance, fixed line-edit size clipping some text on long numbers) Apart from that, there are a couple of minor code formatting issues for which I have left comments too. |
* Could be used by widgets that must reflect the current advanced digitizing state. | ||
* | ||
* \param locked Whether the parameter is locked or not. | ||
*/ |
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.
Doc strings always apply to a single method, so this needs to be copied to every of the 4 methods of the block below. (It's for the generated C++ and Python API docs).
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. It seems doxygen supports one docstring for multiple fcuntions (https://stackoverflow.com/a/37286334) but it doesn't seem used in QGIS... Would make the code more readable IMO.
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 nice.
But it would need some (non trivial) update to sipify (the sip code generator) too. I'm not sure you want to start going down that hole ;)
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, copies the docstring to all methods
@@ -399,6 +472,9 @@ class GUI_EXPORT QgsAdvancedDigitizingDockWidget : public QgsDockWidget, private | |||
*/ | |||
void activateCad( bool enabled ); | |||
|
|||
//! enable/disable floater | |||
void toggleFloater( bool enabled ); |
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 call that setFloaterEnabled
, toggle sounds like it will turn if on and off if called repeatedly without paramter.
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.
removed this method (small refactoring mentionned above)
void QgsAdvancedDigitizingDockWidget::hideEvent( QHideEvent * ) | ||
{ | ||
// disable CAD but do not unset map event filter | ||
// so it will be reactivated whenever the map tool is show again | ||
setCadEnabled( false ); | ||
// TODO : remove this (we don't want to disable CAD events, even when the dockerwidget is hidden) |
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.
What's this TODO exactly?
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.
Removed the whole hideEvent function, since we don't want anymore to disable cad when the dockwidget is closed
{ | ||
if ( !locked ) | ||
{ | ||
mXLineEdit->setStyleSheet( "" ); |
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.
Almost always: ""
-> QString()
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
} | ||
else | ||
{ | ||
mXLineEdit->setStyleSheet( "font-weight: bold" ); |
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.
mXLineEdit->setStyleSheet( "font-weight: bold" ); | |
mXLineEdit->setStyleSheet( QStringLiteral( "font-weight: bold" ) ); |
Also applies to the others below.
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
* | ||
* \param value The current value (as a QString, as it could be an expression). | ||
*/ | ||
void setX( QString value ); |
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 those are all normal public functions and not used as slots (always called from lambdas).
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 thought that sometimes it may be interesting for widgets to use them as slots (e.g. to connect a QEditLine::textChanged). Is there a downside to have them as slots ? Or would it be possible to later on change them as slots without breaking backwards compatilibility ?
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.
The only downside is, that it adds a bit of overhead, so no big deal.
But on the other hand, the advantage is tiny as well: somebody might need a different signature (e.g. double as parameter), and it's very easy to trigger them with lambdas as you did.
Upgrading it to a slot when needed in the future will be trivial. Downgrading not (because it will be stable 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.
Ok, changed to normal functions
Great, thanks for the detailed review ! Will go through the comments, and maybe ask some questions... |
Ok just pushed the changes. It includes all your comments and a little bit of additionnal refactoring. I kept all commits separated to facilitate review but would of course squash before final PR. |
* \note The value is a QString, as it could be an expression. | ||
* \since QGIS 3.8 | ||
*/ | ||
void setX( QString value ); |
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.
Don't forget this
void setX( QString value ); | |
void setX( const QString &value ); |
* Could be used by widgets that must reflect the current advanced digitizing state. | ||
* \since QGIS 3.8 | ||
*/ | ||
void valueXChanged( QString value ); |
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.
Here too
void valueXChanged( QString value ); | |
void valueXChanged( const QString &value ); |
* Could be used by widgets to capture the focus when a field is being edited. | ||
* \since QGIS 3.8 | ||
*/ | ||
void focusOnY(); |
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.
Just an idea: should this be called focusOnYRequested
to make it unambiguously clear that it's a signal (currently the name itself could be interpreted as an imperative method).
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.
Yes that's better, will do the change
Looking good (just one additional comment I left as consideration) |
Great, I'll open the PR then. I forgot to ask for last PR : is there any place where I should mark these contributions as sponsored by the client and/or in partnership with OpenGIS ? And should I mark you as co-author of the commit ? |
Good question. |
PR here : qgis#9988 (I created another branch for the squash) |
Description
Include a few sentences describing the overall goals for this PR (pull request). If applicable also add screenshots.
Checklist
fixes #11111
in the commit message next to the description[FEATURE]
in the commit message[needs-docs]
in the commit message and contain sufficient information in the commit message to be documentedscripts/prepare-commit.sh
script before each commit