Skip to content
This repository has been archived by the owner on Dec 14, 2021. It is now read-only.

feat: menu action for graph 'save as image' #1104

Merged
merged 2 commits into from Nov 18, 2020

Conversation

miltolstoy
Copy link
Contributor

Resolves #426

How to test:

  1. Launch Sourcetrail and open any project
  2. Click on graph (see first issue)
  3. Select "Edit" -> "Save as Image" and choose a path to save
    Expected result: graph image is saved at chosen path
  4. Open another graph in second tab (this project or another one)
  5. Click on graph on newly opened tab (see first issue)
  6. Select "Edit" -> "Save as Image" and choose a path to save
    Expected result: graph from second tab is saved at chosen path

Issues:

  1. Graph should be clicked at least once to be marked as focused. Last focused graph is remembered to trigger saving only for needed one, not for all opened graphs.
  2. exportGraph() direct reuse failed for me, so I copy-pasted dialog creation to QtMainWindow.cpp. The issue was in GLib assertions like this:
(Sourcetrail:23434): GLib-GObject-CRITICAL **: 11:11:44.894: g_value_take_string: assertion 'G_VALUE_HOLDS_STRING (value)' failed
(Sourcetrail:23434): GLib-GObject-WARNING **: 11:11:44.894: ../../../gobject/gtype.c:4268: type id '0' is invalid
(Sourcetrail:23434): GLib-GObject-WARNING **: 11:11:44.894: can't peek value table for type '<invalid>' which is not currently referenced
(Sourcetrail:23434): GLib-GObject-WARNING **: 11:11:44.894: ../../../gobject/gvalue.c:185: cannot initialize GValue with type '(null)', this type has no GTypeValueTable implementation
(Sourcetrail:23434): GLib-GObject-CRITICAL **: 11:11:44.894: g_value_type_compatible: assertion 'src_type' failed
(Sourcetrail:23434): GLib-GObject-CRITICAL **: 11:11:44.894: g_value_copy: assertion 'g_value_type_compatible (G_VALUE_TYPE (src_value), G_VALUE_TYPE (dest_value))' failed

I think that this can be caused by nullptr passing as a parent to QFileDialog. Anyway GLib floods log with errors and filePath is empty in result.

Notes:

  1. Graph image creation at message handling moment causes a segfault or empty image. Seems like that QtGraphicsView object is not in a proper state at that moment. So I added image caching to avoid this.
  2. SVG format excluded for now because of second above-mentioned issue. Just to not copy-paste this logic again to QtMainWindow.cpp.

Questions:

  1. I used last filepicker location for saving dialog initializing directory because this is most probably be project directory. Maybe change it to user home directory? Maybe add a placeholder image name like "${project_name}_${view_name}.png"?
  2. I bound Ctrl+Shift+S hotkey to this action. Is this combination fine and is hotkey needed at all?

Copy link
Contributor

@mlangkabel mlangkabel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @miltolstoy, thanks for working on this feature. I took a look at your changes and I have a few remarks that need to be addressed before I can merge this PR.

  • Please add the new .h file to the cmake of the src/lib folder (i know that it is not necessary for compilation, but it will make this file show up in IDEs like Visual Studio)
  • Please avoid #pragma once. Take a look at our other header files.
  • You may consider moving the new message type to the graph subfolder, because it is just related to the graph
  • Please avoid the static variable in the QtGraphics view. I think that you did this to store the correct graph if the user has multiple tabs open. You could solve this by directing your message only to the QtGraphicsView of the current tab. Hint: See MessageBase::getSchedulerId() and MessageListenerBase::getSchedulerId()
  • Try to reduce code duplication by making the MainWindow just send the message and implement the code that shows the file selection dialog in the QtGraphicsView
  • prepend menber variables with m_ and statics (you shouldn't have any) with s_

@miltolstoy
Copy link
Contributor Author

Hi, @mlangkabel!
Thanks for the review. All comments are fixed except one.

Try to reduce code duplication by making the MainWindow just send the message and implement the code that shows the file selection dialog in the QtGraphicsView

Please check issue number 2 in PR description. If I call QtGraphicsView::exportGraph() in message handler, I will (randomly?) receive one of 2 situations:

  • empty filePath after dialog closed, so no file is saved
  • a bunch of GLib errors and segfault:
(Sourcetrail:31165): GLib-GObject-CRITICAL **: 18:39:24.786: g_value_take_string: assertion 'G_VALUE_HOLDS_STRING (value)' failed

(Sourcetrail:31165): GLib-GObject-WARNING **: 18:39:24.786: ../../../gobject/gtype.c:4268: type id '0' is invalid

(Sourcetrail:31165): GLib-GObject-WARNING **: 18:39:24.786: can't peek value table for type '<invalid>' which is not currently referenced

(Sourcetrail:31165): GLib-GObject-WARNING **: 18:39:24.786: ../../../gobject/gvalue.c:185: cannot initialize GValue with type '(null)', this type has no GTypeValueTable implementation

(Sourcetrail:31165): GLib-GObject-CRITICAL **: 18:39:24.786: g_value_type_compatible: assertion 'src_type' failed

(Sourcetrail:31165): GLib-GObject-CRITICAL **: 18:39:24.786: g_value_copy: assertion 'g_value_type_compatible (G_VALUE_TYPE (src_value), G_VALUE_TYPE (dest_value))' failed

(Sourcetrail:31165): GLib-GObject-CRITICAL **: 18:39:24.786: g_value_type_compatible: assertion 'src_type' failed

(Sourcetrail:31165): GLib-GObject-WARNING **: 18:39:24.786: unable to set property 'text' of type 'gchararray' from value of type '(null)'

@mlangkabel
Copy link
Contributor

Thanks for fixing the issues I mentioned in the review. I think we can pull the code as it is now.

@mlangkabel mlangkabel merged commit 8990eb3 into CoatiSoftware:master Nov 18, 2020
@miltolstoy miltolstoy deleted the 426-save_graph_as_image branch November 18, 2020 17:20
@miltolstoy
Copy link
Contributor Author

Thanks for fixing the issues I mentioned in the review. I think we can pull the code as it is now.

You are welcome. See you in further PRs. =)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Menu action for graph "Save as Image"
2 participants