search

alexzaitsev / apk-dependency-graph

Android class dependency visualizer. This tool helps to visualize the current state of the project.
Apache License 2.0
751 stars 71 forks source link

Toggle visibility of inactive links/nodes #20

Closed WarrenFaith closed 7 years ago

WarrenFaith commented 7 years ago

The following changes will add two checkboxes to toggle the visibility of inactive links or filtered nodes.

In case the inactive nodes are hidden, the inactive links are hidden as well, as otherwise you would see a lot of grey lines without the nodes.

I also made small improvements to the scripts. The changes to the bat script are not tested, I do not have a windows system available.

alexzaitsev commented 7 years ago

Thank you @WarrenFaith, I'll test your changes on Windows.

alexzaitsev commented 7 years ago

@WarrenFaith I tested your changes it and it behaves in unexpected way for me. First of all, if i uncheck 'Show inactive paths' all the paths become unvisible. Also if i uncheck 'Show inactive nodes' - again all paths are unvisible (but filtered nodes are going away so it half-works). Please check screenshots here.

Also I suggest to move output folder to build folder. What do you think about it?

WarrenFaith commented 7 years ago

build seems a bit odd too. Maybe we make it analysed-apk or so?

What you describe is actually the correct behavior. If you have not selected a node, all path are "inactive" as they are just grey. So if you uncheck "show inactive paths" they should be hidden, which they are.

The "show inactive nodes" always hides inactive paths too, which I described above. Reason for that is also that you would see a lot of grey path when you selected a node and all inactive nodes are hidden. There would still be inactive path be drawn between hidden nodes.

alexzaitsev commented 7 years ago

I understood. I have 2 suggestions:

  1. Merge these 2 checkboxes into single one
  2. Show it only if we have selected node
    Does it make sense?
WarrenFaith commented 7 years ago

I could merge it to "show inactive elements". Not sure I understand what you mean by "show it". Do you mean I should show the checkbox only when I have a node selected? I "solved" this by prechecking the checkbox so that you always start with everything visible

alexzaitsev commented 7 years ago

I could merge it to "show inactive elements".

Great, that's exactly what i suggest to do.

Not sure I understand what you mean by "show it". Do you mean I should show the checkbox only when I have a node selected?

Yes, it would be wonderful to make the checkbox itself visible only if user selected any node. It can be confusing otherwise.

WarrenFaith commented 7 years ago

Hm, actually I like the chance to see the nodes without any dependencies or paths. Mostly because my project is so big that it roasts my CPU/GPU during layout procedures :P

alexzaitsev commented 7 years ago

Ok, we can do that but in this case please add some help popup to give user more details. E.g. like here. I just want to be sure that this option won't be confusing for users.

WarrenFaith commented 7 years ago

ok, will update my PR when I get home

alexzaitsev commented 7 years ago

thanks, standing by

WarrenFaith commented 7 years ago

Finally I had some time.

While I checked your changes after my previous PR, I realized that we managed to interpret the "parse the inner classes" flag in a completely different way. That was clearly visible as you changed the help print message in the scripts in a logical way but opposite of its function. I flipped the code and now everything is fine.

Back to this PR: I merged the two input fields and added a tooltip. As I was not able to find a standalone css for "tooltipped", (used by github), I tried materializecss and the results looks pretty cool and modern. Any feedback is appreciated.

alexzaitsev commented 7 years ago

Thanks @WarrenFaith I hope today I'll have some time to review your changes.

alexzaitsev commented 7 years ago

I've made a new release. I tweaked slightly settings block in order to make in more visible over the graph and styled Live Editor button in material way. If you have some time - please review the changes. @WarrenFaith, thanks for you help, I appreciate it!

WarrenFaith commented 7 years ago

The settings background is off by roughly 30 pixels width and height, beside that it looks good (using Chrome on Ubuntu)

alexzaitsev commented 7 years ago

Thanks for feedback. Could you explain a bit more what do you mean by "background is off by roughly 30 pixels width and height"?

WarrenFaith commented 7 years ago

background_issue If I change the width of the .form css to 370px I get it fixed (but imho the bluish default color is a bad choice for that background) background_issue_fixed

alexzaitsev commented 7 years ago

I've fixed it in a way you proposed. Again, thanks. Here is new release: https://github.com/alexzaitsev/apk-dependency-graph/releases/tag/0.1.2