this is a patch for - Bug 361783 - JJ:Add "Show Rulers" checkbox to "Grid and Guides" docker .
Details
First created a showRulers checkbox and then connected it to the view rulers checkbox in menu
Diff Detail
- Repository
- R37 Krita
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Hi, @shreyasr!
Your patch looks nice :) I have tested it and there is one issue: the checkbox is not initialized of the start of Krita if the rullers are enabled in the config. It means there should be some cold initialization in setCanvas().
There is also one minor this, which doesn't look good. I mean the fact that you export the entire Ui::GridConfigWidget from the widget. It would be much more clear and maintainable, if you just added three methods to GridConfigWidget and connected it to the action without accessing internal structures of the widget. Like that:
class GridConfigWidget { ... public: bool showRulers() const; public Q_SLOTS: void setShowRulers(bool value); Q_SIGNALS: void showRulersChanged(); };
That would make the code much more maintainable in the future.
Accedentally publiched the previous review. So just change the status to "needs changes" :)
Hi, @shreyasr!
I'll answer to your IRC questions here :)
- 'cold initialization' usually means that when you connect something to a signal that flags changesof some property, you should also initialize the receiver with the current value of the source to make them sync. It happens because the sender might have already been initialized before and there is no 'changed' signal planned. In a code it usually looks like that:
connect(sender, SIGNAL(valueChanged(bool)), receiver, SLOT(valueChanged(bool))); reciever->setValue(sender->value());
Please also take into account that it is a good practice to make 'cold initialization' after the signal was connected. In your current case it is not that important, but in case of Qt::DirectConnection, not following this rule might cause you a lot of troubles.
- For showRulersChanged(bool) signal, you should just connect it directly to the signal of the checkbox. It will project the interface of the checkbox onto the interface of GridConfigWidget, without revealing the details about the implementation of the widget to the outside world.
connect (checkBox, SIGNAL(toggled(bool)), this, SIGNAL(showRulersChanged(bool)));
Hi, @shreyasr!
The purpose of the signal firing out of the GridConfigWidget is that you can connect it to the action directly.
Basically, you should connect the signal of GridConfigWidget to the action's setChecked() slot, then connect the action's toggled() signal to the slot of GridConfigWidget.
plugins/dockers/griddocker/grid_config_widget.cpp | ||
---|---|---|
335 | As far as I can tell, this explicit emission of the signal is excessive. It should be enough to just call setChecked(). Although it needs to be tested. | |
plugins/dockers/griddocker/grid_config_widget.h | ||
60 | If you add 'bool' parameter to the signal, then you can connect it directly to the action's setChecked() slot. |
Now it looks correct!
If you don't a commit access, please write me your "Name Surname <email.address@example.com>" so put your authorship on your patch
You can send me your data to dimula73@gmail.com