FreeCAD icon indicating copy to clipboard operation
FreeCAD copied to clipboard

Draft toggle grid as a toggle button to indicate grid visibility

Open furgo16 opened this issue 9 months ago • 12 comments

Fixes https://github.com/FreeCAD/FreeCAD/issues/13527

Makes the button for the Draft_ToggleGrid command act as a real toggle button:

  • Raised: the grid is not visible
  • Depressed: the grid is visible

Similar in spirit to the behavior of the Sketcher_Grid command, which is also a toggle button.

Submitting as authored and suggested by @Syres916 at https://github.com/FreeCAD/FreeCAD/issues/13527#issuecomment-2096322545.

furgo16 avatar May 12 '24 08:05 furgo16

Nice improvement, I can confirm that it works properly. If the PR is ready to merge, can you undraft it?

FEA-eng avatar May 12 '24 16:05 FEA-eng

CAn we set the color of this grid already in a preference pack or stylesheet?

MisterMakerNL avatar May 12 '24 20:05 MisterMakerNL

@FEA-eng thank you for testing. Credit (as per commit authorship) goes to Syres916. I intentionally submitted it as draft, as there are a couple of pieces I want to look at to better understand them before requesting a review. However, if anyone wants to test or review it already, that will be welcome too!

@MisterMakerNL I'm not sure I understand the question. Do you mean if the grid itself is themeable? If so, I don't have the answer, this PR deals only with the button to toggle the grid on and off.

furgo16 avatar May 13 '24 01:05 furgo16

Another problem:

  1. Start FreeCAD.
  2. Create a document.
  3. Switch to the Draft WB.
  4. Grid is visible. Grid button appears checked in the toolbar, but not in the status bar.

Also the entry in the Utilities menu is not updated.

Maybe handling this via the action is better. But I am not sure how that can be accomplished. action = Gui.Command.get("Draft_ToggleGrid").getAction()[0]

Roy-043 avatar May 14 '24 13:05 Roy-043

This works (tested with the current dev version, not with the code of this PR):

action = Gui.Command.get("Draft_ToggleGrid").getAction()[0]
action.setCheckable(True)
action.setChecked(True)
action.setChecked(False)
OS: Windows 8 build 9600
Word size of FreeCAD: 64-bit
Version: 0.22.0dev.37302 (Git)
Build type: Release
Branch: main
Hash: 0e24e121eb3e5708c380596e0a9fd583befac977
Python 3.11.9, Qt 5.15.13, Coin 4.0.2, Vtk 9.2.6, OCC 7.7.2
Locale: C/Default (C) [ OS: Dutch/Netherlands (nl_NL) ]
Installed mods:

Roy-043 avatar May 15 '24 18:05 Roy-043

@furgo16 I know this is a new PR but also on this one the heads up for the upcoming feature freeze on 2024-06-03

maxwxyz avatar May 15 '24 18:05 maxwxyz

@Roy-043 thanks. That looks promising, as it sets the button status on all toolbars at once. However, as a quick test I simply added the following to the current _find_grid_toolbutton:

        action = FreeCADGui.Command.get("Draft_ToggleGrid").getAction()[0]
        action.setCheckable(tbButtonEnabled)
        action.setChecked(tbButtonChecked)

I'm not sure if it's the actual cause, but having that last statement (action.setChecked()) results in UI freeze due to maximum recursion depth exceeded.

Traceback (most recent call last):
  File "$HOME/dev/furgo16/FreeCAD/build/Mod/Draft/draftguitools/gui_grid.py", line 91, in Activated
    WorkingPlane.get_working_plane()
  File "$HOME/dev/furgo16/FreeCAD/build/Mod/Draft/WorkingPlane.py", line 1752, in get_working_plane
    wp._update_all(_hist_add=False)
  File "$HOME/dev/furgo16/FreeCAD/build/Mod/Draft/WorkingPlane.py", line 1687, in _update_all
    self._update_grid()
  File "$HOME/dev/furgo16/FreeCAD/build/Mod/Draft/WorkingPlane.py", line 1724, in _update_grid
    FreeCADGui.Snapper.setGrid()
  File "$HOME/dev/furgo16/FreeCAD/build/Mod/Draft/draftguitools/gui_snapper.py", line 1585, in setGrid
    self.setTrackers()
  File "$HOME/dev/furgo16/FreeCAD/build/Mod/Draft/draftguitools/gui_snapper.py", line 1590, in setTrackers
    v = Draft.get3DView()
  File "$HOME/dev/furgo16/FreeCAD/build/Mod/Draft/draftutils/gui_utils.py", line 67, in get_3d_view
    import FreeCADGui as Gui
RecursionError: maximum recursion depth exceeded
Running the Python command 'Draft_ToggleGrid' failed, try to resume 

furgo16 avatar May 16 '24 17:05 furgo16

Try removing: "Checkable": False, From gui_grid.py.

Roy-043 avatar May 17 '24 12:05 Roy-043

Update after a quick test:

  1. The code does not handle multiple documents properly (review comment) => can reproduce
  2. Toolbar and status bar buttons out of sync (review comment) => cannot reproduce
  3. Also the entry in the Utilities menu is not updated (review comment) => cannot reproduce

I'm not sure where to start looking to address the first point. For the rest and general testing, if someone who's interested in this feature wants to give this a go and report back, feedback will be welcome.

furgo16 avatar May 17 '24 16:05 furgo16

Switching to the action solution has indeed fixed some issues.

I don't think there is a need to change gui_grid.py. This may have been required before though.

The references to the Draft Tray and the draftToolBar (which both point to the same GUI element) are not relevant. The code does not interact with the Tray.

_update_grid_gui: The function needs checking, this apart from the multi-document issue:

  • gui_utils.get_3d_view() is used as a check twice. The 2nd check is superfluous.
  • If there is a 3D view it follows that the GUI is up. The check FreeCAD.GuiUp is therefore strange.

_set_grid_button_state: action.setCheckable(button_enable) can just become action.setCheckable(True)

It may be worth considering merging the code with WorkingPlane.py. We can then use a single view observer. And there is the intention to make the grid a property of the working plane in the future.

I can help with the multi-document issue if you want. Either with a pointer (look there...) or with code. Just let me know.

Roy-043 avatar May 18 '24 10:05 Roy-043

I don't think there is a need to change gui_grid.py. This may have been required before though.

  1. I'll double check that.

The references to the Draft Tray and the draftToolBar (which both point to the same GUI element) are not relevant. The code does not interact with the Tray.

  1. Understood. I had just left the comment in there, since the observer code on this PR was originally based on that other one.

_update_grid_gui: The function needs checking, this apart from the multi-document issue:

gui_utils.get_3d_view() is used as a check twice. The 2nd check is superfluous. If there is a 3D view it follows that the GUI is up. The check FreeCAD.GuiUp is therefore strange.

  1. I'll double-check that too.

It may be worth considering merging the code with WorkingPlane.py. We can then use a single view observer. And there is the intention to make the grid a property of the working plane in the future.

  1. That makes sense. Given the time, my (limited) current skills and knowledge of the FreeCAD codebase, I'd do this on the next iteration on a separate PR.

I can help with the multi-document issue if you want. Either with a pointer (look there...) or with code. Just let me know.

Thanks. We can start with the "teach a man how to fish" approach with pointers, and I can ask for further help where I get stuck. That said, I'll be cutting down my contribution time in the next few weeks, but I'll do my best to wrap up this PR before the 1.0 feature freeze.

furgo16 avatar May 22 '24 07:05 furgo16

OK, the pointer then :wink:

FreeCADGui.Snapper.grid is not updated as you switch views. It contains the grid belonging to the view that was active when the last Draft command was executed. To get access to the grid belonging to a view you need to use the FreeCADGui.Snapper.trackers list. The setTrackers function shows how this list is constructed.

https://github.com/FreeCAD/FreeCAD/blob/1da8e17ffa67d9bb4c00c75378066dab030b5204/src/Mod/Draft/draftguitools/gui_snapper.py#L1588

Roy-043 avatar May 22 '24 13:05 Roy-043

Sorry for the noise, git rebase gone wrong. Will try to fix it.

furgo16 avatar Jun 02 '24 07:06 furgo16

I'm closing this in favor of https://github.com/FreeCAD/FreeCAD/pull/14452 to avoid the git rebase becoming a time sink. The new PR keeps the original commits from @Syres916 to retain the credit from those contributions.

furgo16 avatar Jun 02 '24 13:06 furgo16