[Draft] Snap statusbar #3228
[Draft] Snap statusbar #3228
Conversation
|
Is this supposed to be a new version of #3220? In general you don't need to close the previous pull request. You can just edit it as you want, add and remove commits, and then just force push to update everything. You can add, remove, and re-write commits with Also, just a reminder, if you are adding new files, you have to include them in the |
|
@vocx-fc I agree, but i'm not so expert and i made a bit of a mess, so i thought it was better to start from scratch. |
|
Ok, no problem. I think in this pull request you don't add new files, but in #3167 you do, so I was just talking in general. |
|
I will probably add a new dialog for preferences, but i'll finish it when I'll be sure it is the right way to go and will be merged after. That's why this is still a Draft PR. |
carlopav
force-pushed
the
SnapStatusbar
branch
4 times, most recently
from
bd12939
to
c05f4ed
Compare
|
To be merged after #3167. |
|
@yorikvanhavre |
|
Ok your turn now :) Mind to have a look at the merge conflicts @carlopav ? |
|
done, by the way @vocx-fc, we should also change the super() methods here, isn't it? |
|
Where exactly? In the various Draft Snap commands? This is done in my branch #3344. Basically, at the end I add the appropriate super for all commands that were moved. |
Changed snap toolbar behaviour: - create a list of available snaps (Gui.Snapper.snaps) - make it consistent with Snap Gui Commands (in gui_snaps module) - create a list of active snaps (Gui.Snapper.active_snaps) - refactor the isEnabled() method to allow it to check if the given snap is in Gui.Snapper.active_snaps and not if the snap toolbar button isChecked() - updated and reordered the new list of gui snap commands in draftutils.init_tools and used it as a base to refactor the creation of draft toolbar - updated all the draft snap gui tools to make them control the toolbar buttons directly . . .
[Draft] Attempt to make base classes compatible with super on py2 [Draft] Refactored imports of snapper and snaps gui tools . .
|
@yorikvanhavre Ufff that rebase was definitely hard. I also corrected the super according to @vocx-fc recent findings :) |
| @@ -1893,8 +1893,7 @@ def getXPM(self,iconname,size=16): | |||
| return str(a) | |||
|
|
|||
| def togglesnap(self): | |||
| if hasattr(FreeCADGui,"Snapper"): | |||
| FreeCADGui.Snapper.toggle() | |||
| FreeCADGui.doCommand('FreeCADGui.runCommand("Draft_Snap_Lock")') | |||
There was a problem hiding this comment.
Is this the best way of doing it? I am not entirely sure what class calls which. To run this command, it must be defined first, but DraftGui is one of the early modules loaded. So, it's like calling something that will be defined later.
If this is the case, then DraftGui must be loaded after the gui_snaps commands are loaded.
As I can see, Draft_Snap_Lock basically calls Gui.Snapper.masterbutton.toggle(), so it looks very similar to Gui.Snapper.toggle().
Anyway, it's just a comment. As I said, DraftGui is quite complex because it is both the DraftToolBar, the snap toolbar, and the task panel interface for most commands.
There was a problem hiding this comment.
As I can see, Draft_Snap_Lock basically calls Gui.Snapper.masterbutton.toggle(), so it looks very similar to Gui.Snapper.toggle().
This was the previous behaviour. With proposed changes Draft_Snap_Lock just calls Gui.Snapper.toggle_snap('Lock') and sync the interface (toolbar and statusbar).
I wish the default behaviour to always interact with Snapper through Snaps commands to set the active_snaps. (EDIT and with current PR also Draft_Snap_Lock became exactly as other snaps seen from outside the Snapper)
Do you think we should modify something in the order the modules are loaded?
There was a problem hiding this comment.
I'm just not sure how it works really. I've never investigated deeply this behavior. But ideally we should try split DraftGui in discrete parts as much as possible.
| @@ -32,21 +32,28 @@ | |||
| # This module provides tools to handle point snapping and | |||
| # everything that goes with it (toolbar buttons, cursor icons, etc.). | |||
|
|
|||
| import FreeCAD as App | |||
There was a problem hiding this comment.
Please notice that we want to import first the generic modules, that is, modules provided by Python itself, or Python packages; at the end we want to import our own modules, meaning FreeCAD et al.
This is the reason I placed the import of FreeCAD after the Qt imports. Qt imports are more generic, while FreeCAD, Draft, DraftVecUtil, etc., are more specific.
There was a problem hiding this comment.
ok, i thought FreeCAD had to be the first one... no problem to change this.
| @@ -75,6 +82,7 @@ class Snapper: | |||
| """ | |||
|
|
|||
| def __init__(self): | |||
|
|
|||
There was a problem hiding this comment.
We don't need a new line here because we don't have docstring. The docstring was already placed in the class definition above.
| @@ -191,14 +236,12 @@ def snap(self, screenpos, | |||
|
|
|||
| self.running = True | |||
|
|
|||
| global Part, DraftGeomUtils | |||
| import Part, DraftGeomUtils | |||
There was a problem hiding this comment.
The reason the modules are placed in this function is because we don't want to globaly import Part when we import the snapper. According to Yorik this makes loading of Draft quite slow, so for the time being, I would leave these imports here. Maybe I'd remove the global keyword.
To be honest I didn't check this thoroughly because I was doing many things. But eventually I need to test this better.
| if o and (self.isEnabled('extension') | ||
| or self.isEnabled('parallel')): | ||
| ob = FreeCAD.ActiveDocument.getObject(o) | ||
| for o in [self.lastObj[1], self.lastObj[0]]: |
There was a problem hiding this comment.
Just nitpicking here but you'll see that in various iterations when you search for something in a list, it is theoretically faster to search in a tuple than in a list.
So, in many cases I use for obj in (...) rather than for obj in [...]. Apparently there is tiny difference.
There was a problem hiding this comment.
The way I heard is that lists are mutable (you can modify an element of the list after it has been created, while tuples not. So when creating a tuple, the exact amount of memory is allocated to it, while for lists there is more memory allocated because it might grow later on, and they are much less efficient. So the idea is always use tuples when you know your list won't need to be modified later on
| p = Vector(info['x'], info['y'], info['z']) | ||
|
|
||
| def snapToVertex(self,info,active=False): | ||
| p = App.Vector(info['x'],info['y'],info['z']) |
| if self.isEnabled("passive"): | ||
| return [p, 'endpoint', self.toWP(p)] | ||
| if self.isEnabled("Near"): | ||
| return [p,'endpoint',self.toWP(p)] |
|
|
||
| if (Draft.getType(obj) == "Wall"): | ||
| # special snapping for wall: snap to its base shape if it is linear | ||
| if obj.Base: | ||
| if not obj.Base.Shape.Solids: | ||
| for v in obj.Base.Shape.Vertexes: | ||
| snaps.append([v.Point, 'special', self.toWP(v.Point)]) |
There was a problem hiding this comment.
I think here you have to reduce the indentation.
| def getScreenDist(self, dist, cursor): | ||
| """Return a distance in 3D space from a screen pixels distance.""" | ||
| view = Draft.get3DView() | ||
| p1 = view.getPoint(cursor) | ||
| p2 = view.getPoint((cursor[0] + dist, cursor[1])) | ||
| return (p2.sub(p1)).Length | ||
|
|
||
|
|
There was a problem hiding this comment.
These methods are inside a class, so they only need one newline of space between them. For methods outside of a class, that is, functions, the spacing is two empty newlines.
| plane=FreeCAD.DraftWorkingPlane, | ||
| mask=FreeCADGui.Snapper.affinity) | ||
| plane = App.DraftWorkingPlane, | ||
| mask = App.Snapper.affinity) |
There was a problem hiding this comment.
These are key=value arguments inside a function. In this case, there should not be a space around the equals sign.
functions(key=value, one="string")The space is needed outside of function arguments.
result = function()
b = second_f(key=result, another_key="some string")
| b.setText(QtCore.QCoreApplication.translate("Draft_Snap", "Snap " + gc[11:])) | ||
| b.setToolTip(QtCore.QCoreApplication.translate("Draft_Snap", "Snap " + gc[11:])) | ||
| b.setObjectName(gc + button_suffix) | ||
| b.setWhatsThis("Draft_"+gc[11:].capitalize()) |
| b.setChecked(state) | ||
| # restore toolbar buttons state | ||
| if snap_modes: | ||
| for a in toolbar.findChildren(QtGui.QAction): |
There was a problem hiding this comment.
I suggest you use better variables than a or b. Maybe action or button?
| """retuns snap toolbar object""" | ||
| mw = Gui.getMainWindow() | ||
| if mw: | ||
| toolbar = mw.findChild(QtGui.QToolBar,"Draft Snap") |
| self.savedButtonStates.append(self.toolbarButtons[i].isChecked()) | ||
| self.toolbarButtons[i].setEnabled(False) | ||
| self.saveSnapModes() | ||
| "toggle FreeCAD Draft Grid" |
There was a problem hiding this comment.
Triple quotes and capital letter for docstring. It should end in a dot.
"""Toggle Draft grid."""
| return False | ||
|
|
||
| def isEnabled(self, snap): | ||
| "Returns true if the given snap is on" |
There was a problem hiding this comment.
Imperative mood.
"""Return true if the given snap is on."""
|
|
||
|
|
||
| def toggle_snap(self, snap, set_to = None): | ||
| "Sets the given snap on/off according to the given parameter" |
There was a problem hiding this comment.
def toggle_snap(self, snap, set_to=None):
"""Set the given snap on/off according to the given parameter."""
| def setGrid(self): | ||
| """Set the grid, if visible.""" | ||
| self.setTrackers() | ||
| if self.grid and (not self.forceGridOff): | ||
| if self.grid.Visible: | ||
| self.grid.set() | ||
|
|
||
|
|
There was a problem hiding this comment.
No need for extra newline.
Cleanup after @vocx-fc review
| import FreeCADGui as Gui | ||
| import draftguitools.gui_base as gui_base | ||
|
|
||
| from PySide import QtGui | ||
| from PySide.QtCore import QT_TRANSLATE_NOOP |
There was a problem hiding this comment.
Again these imports should occur first because they are not specific to FreeCAD.
|
|
||
|
|
||
| def get_snap_statusbar_widget(): | ||
| """retuns snap statusbar button""" |
|
|
||
|
|
||
| def sync_snap_toolbar_button(button, status): | ||
| """set snap toolbar button to given state""" |
There was a problem hiding this comment.
Capital letter at the beginning, and period at the end.
|
|
||
| import FreeCAD as App | ||
| import FreeCADGui as Gui | ||
| from PySide import QtGui | ||
| from PySide import QtCore, QtGui |
There was a problem hiding this comment.
One imported module per line.
| ) | ||
| custom_scale = QtGui.QInputDialog.getText(None, "Set custom scale", dialog_text) | ||
| custom_scale = QtGui.QInputDialog.getText(None, "Set custom scale", | ||
| dialog_text) | ||
| if custom_scale[1]: | ||
| print(custom_scale[0]) | ||
| scale = label_to_scale(custom_scale[0]) | ||
| if scale is None: return |
There was a problem hiding this comment.
The return must be in its own line.
| gridbutton.setFlat(True) | ||
| QtCore.QObject.connect(gridbutton,QtCore.SIGNAL("clicked()"), | ||
| lambda f=Gui.doCommand, | ||
| arg='Gui.runCommand("Draft_ToggleGrid")':f(arg)) |
There was a problem hiding this comment.
I'm not sure of the syntax here. Is it supposed to be
Gui.doCommand('Gui.runCommand("Draft_ToggleGrid")')
This looks convoluted. I think you mean to run just Gui.runCommand("Draft_ToggleGrid").
There was a problem hiding this comment.
well, the fact is that I wanted the string Gui.runCommand("Draft_ToggleGrid") to be printed into the console... like if you were activating the command straight from the UI.
| 'Draft_Snap_Intersection', 'Draft_Snap_Perpendicular', | ||
| 'Draft_Snap_Extension', 'Draft_Snap_Parallel', | ||
| 'Draft_Snap_Special', 'Draft_Snap_Near', | ||
| 'Draft_Snap_Ortho', 'Draft_Snap_Grid', |
There was a problem hiding this comment.
Many of these lines have whitespace at the end that must be removed.
| from draftutils.translate import _tr | ||
|
|
||
|
|
||
| # UTILITIES ----------------------------------------------------------------- | ||
|
|
||
|
|
||
| def get_snap_statusbar_widget(): | ||
| """retuns snap statusbar button""" | ||
| """Retuns snap statusbar button.""" |
There was a problem hiding this comment.
"""Return snap statusbar button."""
There was a problem hiding this comment.
My bad, I'll correct it now!
thanks to vocx revisions
|
Thanks for the hard work and extensive reviewing guys! Merging now |
|
Yuppi!!! |
Thank you for creating a pull request to contribute to FreeCAD! To ease integration, please confirm the following:
git pull --rebase upstream master./bin/FreeCAD --run-test 0issue #<id>orfixes #<id>where<id>is the associated MantisBT issue id if one existsAnd please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 0.19 Changelog Forum Thread.