Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add expandable layer properties #68

Merged
merged 29 commits into from
Nov 25, 2018

Conversation

sofroniewn
Copy link
Contributor

@sofroniewn sofroniewn commented Nov 20, 2018

What does this PR do? Why are doing this? References?

This PR adds layer specific properties back to the layer widgets. They are accessible by double clicking the layer widget. The layer widget then expands in place to reveal the extra properties.

This PR is still in progress, but I thought I'd give people a chance to check it out now and weigh in on some of the stylistics aspects before going too far. I feel right now it just looks a little ugly with the text labels and layer name not lining up and the spacing not being regular. I also havn't implemented all the possible layer properties yet, like the symbol for markers, and the interpolation mode for images. I'm also thinking about adding a text edit box that only accepts numbers next to the opacity slider so you can type in a value too.

A final note, this PR also contains the edits from the drag / drop PR and the delete button PR as well.

image

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

[Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.]

  • TestCase Run examples/layers.ipynb and manually interact with gui

Final Checklist:

  • My PR is the possible minimum work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@sofroniewn sofroniewn added feature New feature or request in-progress labels Nov 20, 2018
@sofroniewn sofroniewn requested a review from a team November 20, 2018 19:23
@pep8speaks
Copy link

pep8speaks commented Nov 20, 2018

Hello @sofroniewn! Thanks for updating the PR.

Line 67:5: E301 expected 1 blank line, found 0
Line 68:5: E301 expected 1 blank line, found 0
Line 69:5: E301 expected 1 blank line, found 0
Line 70:5: E301 expected 1 blank line, found 0
Line 71:5: E301 expected 1 blank line, found 0

Line 5:1: E302 expected 2 blank lines, found 1
Line 15:80: E501 line too long (85 > 79 characters)
Line 22:80: E501 line too long (87 > 79 characters)
Line 25:80: E501 line too long (100 > 79 characters)

Line 1:80: E501 line too long (122 > 79 characters)
Line 7:24: E231 missing whitespace after ','
Line 7:32: E231 missing whitespace after ','
Line 9:1: E302 expected 2 blank lines, found 1
Line 13:80: E501 line too long (129 > 79 characters)
Line 14:80: E501 line too long (133 > 79 characters)
Line 21:80: E501 line too long (87 > 79 characters)
Line 27:9: E265 block comment should start with '# '
Line 35:80: E501 line too long (83 > 79 characters)
Line 41:9: E265 block comment should start with '# '
Line 47:80: E501 line too long (82 > 79 characters)
Line 51:80: E501 line too long (85 > 79 characters)
Line 100:24: E225 missing whitespace around operator
Line 100:80: E501 line too long (102 > 79 characters)
Line 142:54: E231 missing whitespace after ','
Line 144:54: E231 missing whitespace after ','
Line 147:5: E303 too many blank lines (2)
Line 152:9: E265 block comment should start with '# '
Line 153:9: E265 block comment should start with '# '
Line 154:9: E265 block comment should start with '# '

Line 5:1: E302 expected 2 blank lines, found 1
Line 11:9: E265 block comment should start with '# '
Line 54:63: E231 missing whitespace after ','
Line 55:61: E231 missing whitespace after ','
Line 74:80: E501 line too long (86 > 79 characters)
Line 78:80: E501 line too long (100 > 79 characters)
Line 98:80: E501 line too long (100 > 79 characters)
Line 114:24: E225 missing whitespace around operator
Line 117:29: E231 missing whitespace after ','

Line 3:80: E501 line too long (82 > 79 characters)
Line 8:28: E231 missing whitespace after ','
Line 8:36: E231 missing whitespace after ','
Line 10:1: E302 expected 2 blank lines, found 1
Line 21:1: E302 expected 2 blank lines, found 1
Line 32:80: E501 line too long (85 > 79 characters)
Line 33:80: E501 line too long (87 > 79 characters)
Line 34:80: E501 line too long (88 > 79 characters)
Line 54:1: E302 expected 2 blank lines, found 1

Line 7:1: E302 expected 2 blank lines, found 1
Line 14:9: E265 block comment should start with '# '
Line 31:12: E111 indentation is not a multiple of four
Line 32:80: E501 line too long (82 > 79 characters)
Line 34:12: E111 indentation is not a multiple of four
Line 35:80: E501 line too long (99 > 79 characters)
Line 42:12: E111 indentation is not a multiple of four
Line 43:80: E501 line too long (82 > 79 characters)
Line 45:12: E111 indentation is not a multiple of four
Line 46:80: E501 line too long (99 > 79 characters)
Line 53:12: E111 indentation is not a multiple of four
Line 54:80: E501 line too long (80 > 79 characters)
Line 56:12: E111 indentation is not a multiple of four
Line 57:80: E501 line too long (100 > 79 characters)

Line 8:1: E303 too many blank lines (3)
Line 19:9: E265 block comment should start with '# '
Line 23:9: E265 block comment should start with '# '

Line 20:1: E302 expected 2 blank lines, found 1
Line 45:1: W293 blank line contains whitespace

Line 14:1: E302 expected 2 blank lines, found 1

Comment last updated on November 23, 2018 at 06:28 Hours UTC

@sofroniewn sofroniewn mentioned this pull request Nov 20, 2018
9 tasks
@freeman-lab
Copy link
Contributor

small aesthetic comment -- i'd move the trash icon to the lower right :) that's where it is on the mac desktop, in photoshop, and a few other tools i know

@jni
Copy link
Member

jni commented Nov 21, 2018

I love the idea, and except for the alignment issue that you already flagged and the naming issue that I've flagged before (it's unclear that I can change the layer names), I think it looks great!

Incidentally, default names could be improved to (a) include the layer type (so add_markers would default to markers, not layer), and (b) add an incrementing integer for each layer (optionally only counting layers of that type).

I also agree with @freeman-lab that the trash icon would be way better at the bottom right, though I don't know why. =D

@jni
Copy link
Member

jni commented Nov 21, 2018

PS Thanks for including a screenshot, so useful. =)

Copy link
Member

@kne42 kne42 left a comment

Choose a reason for hiding this comment

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

Looks beautiful, thank you Nick!

I agree with Juan and Jeremy, I think it would make the most sense to move the trash icon into the lower right corner to separate it in functionality from other utilities that you may want to place up there (e.g. a "new layer" button).

I also agree with Juan surrounding layer naming, but I think we can address that in a future PR.

Other than that, my only other comment is that it seems a bit weird to me how a lot of the existing layout "shifts" when you expand the layer (e.g. the layer name moves left to align with the other properties).

@AhmetCanSolak
Copy link
Contributor

Incidentally, default names could be improved to (a) include the layer type (so add_markers would default to markers, not layer), and (b) add an incrementing integer for each layer (optionally only counting layers of that type).

I love to see option (b). An easy feature indeed. @sofroniewn if you want i can address that after merge of this?

Copy link
Contributor

@AhmetCanSolak AhmetCanSolak left a comment

Choose a reason for hiding this comment

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

Everything seems good aside from the conflicts. I think it is ready to be merged after resolving conflicts.

@sofroniewn
Copy link
Contributor Author

sofroniewn commented Nov 22, 2018

I've moved the trash icon to the lower right. I had initially put it on top as new layers are created up there, so thought it would be easier to have it closer, but agree most other apps have it down there. As to other icons like new layer buttons etc. I was imaging them in a row along with the trash icon too, but that can be at the bottom too.

I've also changed the default layer names for the different classes, but havn't implemented the numerical counter yet. Happy to sync with @AhmetCanSolak on that in a future PR. I think we should also allow layer names to be passed as input arguments to the add_layers functions.

As to the editing of the layer name, agreed that right now it is hard to discover that you can do it, I find the white edit box background that's default quite jarring, maybe we can find some middleground when we redo all the stylings.

I'm going to work now on sorting out some of the alignment problems and extra properties, and then resolve the conflicts. I'll post another screen shot when I think it's ready to go!

@sofroniewn
Copy link
Contributor Author

Also noting here that I closed #65 that just had the trash icon code, which is also contained here

@sofroniewn
Copy link
Contributor Author

Have added some extra properties to the image / marker layers. Definitely still a couple more that could be added, but this seemed like a reasonable place to stop. Didn't add any edit boxes next to the sliders to type in numbers but could be done in the future.

I tried to fix some of the alignment sizing issues. I think it's passable now, but could be improved when we revisit stylings more generally. I'm tempted to make the layer's panel a fixed width that cannot be adjusted - it might be cleaner.

Here's an updated screenshot
image
I think if everyone approves then it's ready to merge

@royerloic
Copy link
Member

Looks great. Concerning the style I think we could reduce margins to have a slightly more compact layout (particularly for the layers panel). Otherwise, I think it is great!

@sofroniewn
Copy link
Contributor Author

@royerloic sounds reasonable, but something that will probably wait until we make a big push on the stylings if that's ok

@jni
Copy link
Member

jni commented Nov 23, 2018

@sofroniewn this looks so great! Re the text styling, it can definitely wait until we're making a concerted style push, but here's my suggestion:

  • frame around it with background matching the surrounding
  • when you click on it, the background becomes white and you're editing
  • when you press enter or click out, the rename is executed and the background goes back to matching the surrounds.

Copy link
Contributor

@AhmetCanSolak AhmetCanSolak left a comment

Choose a reason for hiding this comment

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

Thank you @sofroniewn

@AhmetCanSolak AhmetCanSolak merged commit 6e15cf8 into napari:master Nov 25, 2018
@kne42
Copy link
Member

kne42 commented Nov 25, 2018

@AhmetCanSolak please squash-merge big pull requests like this in the future to avoid cluttering the git history.

@AhmetCanSolak
Copy link
Contributor

Missed that @kne42, i presumed he squashed on his branch. It is not deep yet. If we have #68 and #70 we can revert these two PRs and merge back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants