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 File Browser Styling #4085

Merged
merged 13 commits into from Jan 5, 2018

Conversation

Projects
None yet
5 participants
@Mark-Agent003
Member

Mark-Agent003 commented Dec 30, 2017

This is the Pull Request for #4070 .

The file browser will be more responsive to hovering and clicking with this, but still styled in the default theme.

What it adds:

  • background of item changes when mouse hovers over item [Gray]
  • background of directory item changes when directory is open [Green]
  • if an item is clicked, its background color is changed [Green]

I removed the indenting when selecting file items.

@tresf

This comment has been minimized.

Member

tresf commented Dec 31, 2017

@Mark-Agent003, thanks for this feature. Please fixup the description to describe the enhancement a bit better.

I've tested this and there seems to be some strange behavior when navigating between folders, the old one doesn't seem to get unselected. I've attached a GIF.

Note, I'd also like @Umcaruje to approve the theme/css side of this, since he's the remaining half of the 1.2.0 theme while Rebecca is away. At a glance, the colors look great.

untitled

@tresf

This comment has been minimized.

Member

tresf commented Dec 31, 2017

@Mark-Agent003 the caret background is fixed now, but still leaving the old item selected. Screenshot attached.

screen shot 2017-12-31 at 3 50 56 pm

@tresf

This comment has been minimized.

Member

tresf commented Jan 1, 2018

It's still allowing the old folder to be highlighted when a file from a different folder has been selected.
screen shot 2017-12-31 at 9 53 26 pm

@Mark-Agent003

This comment has been minimized.

Member

Mark-Agent003 commented Jan 1, 2018

Yes, if a directory is open, it will always have that background.

@Mark-Agent003

This comment has been minimized.

Member

Mark-Agent003 commented Jan 1, 2018

A background will change to green if

  1. It is any opened directory or 2. It is an item (not directory) that has been clicked
@tresf

This comment has been minimized.

Member

tresf commented Jan 1, 2018

Yes, if a directory is open, it will always have that background.

Can you please offer another application that behaves this way for comparison? This seems incorrect to me.

@Mark-Agent003

This comment has been minimized.

Member

Mark-Agent003 commented Jan 1, 2018

This seems incorrect to me.

I don't think it's a matter of what's correct as much as it's a matter of what's useful. If it doesn't seem useful to anybody or if it's distracting, it can definitely be removed.

@tresf

This comment has been minimized.

Member

tresf commented Jan 1, 2018

Although I agree -- usefulness must prevail -- file browsing is something computer users are already familiar with; do often enough. Correctness must weigh in to the decision. So in this case, ignoring defacto standards must be justified, not the other way around.

@Mark-Agent003

This comment has been minimized.

Member

Mark-Agent003 commented Jan 1, 2018

I see. I'd like to see what other's think just before I go ahead and remove it.

image

👍 - Keep the open directory styling
👎 - Remove the open directory styling

@tresf

This comment has been minimized.

Member

tresf commented Jan 2, 2018

Sorry, there is some confusion confusion. The open directory highlighting is fine, you are correct.

What I was illustrating in my screenshot is a bug with the implementation where the previous folder was not deselected.

@Mark-Agent003

This comment has been minimized.

Member

Mark-Agent003 commented Jan 2, 2018

So, what you’re saying is: you’ve opened two folders and when you click an item in one, you don’t want the other folder to still be highlighted green?

@tresf

This comment has been minimized.

Member

tresf commented Jan 2, 2018

So, what you’re saying is: you’ve opened two folders and when you click an item in one, you don’t want the other folder to still be highlighted green?

Yes, correct. I would only expect the currently highlighted item (and in this case, also it's parent items) to be highlighted. This is fairly consistent with most parent/child relationships I'm familiar with.
screen shot 2018-01-01 at 9 23 01 pm

screen shot 2018-01-01 at 9 24 32 pm

@Mark-Agent003

This comment has been minimized.

Member

Mark-Agent003 commented Jan 2, 2018

Okay, right now, any open folder is highlighted. Otherwise it can’t be done with just css.

@teeberg

This comment has been minimized.

Member

teeberg commented Jan 2, 2018

aren't the arrow in front of the folder name that points right or down, as well as the next level of that folder being indented, enough indication that it's currently open/expanded? In this comment's screenshot: #4085 (comment), I would personally prefer the right. Just looking at a random image from google of the windows explorer, this seems to be the default behavior at least under windows:

image

@Mark-Agent003

This comment has been minimized.

Member

Mark-Agent003 commented Jan 2, 2018

Another question, does anyone prefer the hover/click highlights to extend all the way on either side?
image
One problem is that it covers up the arrows in the margins.
image

@tresf

This comment has been minimized.

Member

tresf commented Jan 2, 2018

@Mark-Agent003 if you're interested in touching the C++ code, you should be able to recursively force the style upon its parents.

If not, we seem to have hit a CSS limitation inside of Qt. The browser allows this through the > selector, although I cannot seem to get this working with QTreeView::item.

@teeberg this seems consistent with Mac as well.
screen shot 2018-01-01 at 10 24 08 pm

@Mark-Agent003

This comment has been minimized.

Member

Mark-Agent003 commented Jan 2, 2018

@tresf, unfortunately, I am very unfamiliar with the lmms code

@tresf

This comment has been minimized.

Member

tresf commented Jan 2, 2018

No worries. Let's just do child highlighting for now. It solves the original bug report without custom code.

@Mark-Agent003

This comment has been minimized.

Member

Mark-Agent003 commented Jan 2, 2018

Ok, what do you think about the extended hightlights?

@tresf

This comment has been minimized.

Member

tresf commented Jan 2, 2018

Covering up the carets is a non-starter.

@Mark-Agent003

This comment has been minimized.

Member

Mark-Agent003 commented Jan 2, 2018

I can get around that, but it'd require adding open and closed bracket images to the theme.
image

@tresf

This comment has been minimized.

Member

tresf commented Jan 3, 2018

That seems like overkill. Frankly I feel this PR should have been a few CSS lines, merged, and on to the next enhancement. I have no investment one style over the other. @Umcaruje please make a decision here.

@Sawuare

This comment has been minimized.

Member

Sawuare commented Jan 3, 2018

QTreeView  {
	selection-background-color: transparent;
}

QTreeWidget::item:hover  {
	background-color: #3C444E;
}

QTreeView::item:selected {
	background-color: #17793b;
}

hovered image

These lines highlight the hovered item in gray and the selected item in green.
I think they are all we need in this PR.

@Mark-Agent003

This comment has been minimized.

Member

Mark-Agent003 commented Jan 3, 2018

There are two options:
image

Option one is simpler and is CSS-only, simply highlighting and selecting.
Option two adds the open/closed folder brackets as theme-able images in order to allow for the extent of the hover/select highlights into the margins.
@tresf doesn't seem to mind either way, but @Sawuare seems to like the simple option.

@tresf

This comment has been minimized.

Member

tresf commented Jan 4, 2018

@Mark-Agent003 although it's a noble effort to get consensus, I'll ask one final time to please execute the first option, we'll merge, we can close out #4070.

Mark-Agent003 added some commits Jan 4, 2018

@Umcaruje

This comment has been minimized.

Member

Umcaruje commented Jan 4, 2018

I find the 2nd option aesthetically pleasing and I don't see it as too much CSS. I have some inputs on the CSS though.


I don't know why you changed the font size, I feel this should be a part of a seperate issue, and not in this pull request.
The CSS can be reduced a lot, as you can apply the same rule to multiple elements:

QTreeWidget::item:selected,
QTreeWidget::branch:selected {
 	background-color: #17793b;
 }

The indentation before the curly brackets should be one space, not two.
And QTreeView should go before QTreeWidget::item for consistency.

In the end the changes should look like this:

QTreeView {
	outline: none;
	selection-background-color: transparent;
	selection-color: #d1d8e4;
}

QTreeWidget::item {
	padding: 0px;
}

QTreeWidget::item:hover,
QTreeWidget::branch:hover {
	background-color: #3C444E;
}

QTreeWidget::item:selected,
QTreeWidget::branch:selected {
	background-color: #17793b;
}

QTreeView::branch:has-children:open {
	border-image: url("resources:open_branch.png") 0;
}

QTreeView::branch:has-children:closed {
	border-image: url("resources:closed_branch.png") 0;
}

Also, I'm not entirely sure what selection-background-color and selection-color properties do, would you care to explain?

@Mark-Agent003

This comment has been minimized.

Member

Mark-Agent003 commented Jan 4, 2018

@Umcaruje , I added the selection-color to make sure the font color of an item didn't change when a user clicked on it. The background-selection-color effects the background color of the margin when selected:
image

(set to blue)

@Umcaruje

This comment has been minimized.

Member

Umcaruje commented Jan 4, 2018

Ah, thanks for explaining, do you know why the icons change their color from white to grey when they are clicked on? Is this some kind of weird Qt behaviour?

@tresf

This comment has been minimized.

Member

tresf commented Jan 4, 2018

A picture for reference of @Umcaruje's question:

screen shot 2018-01-03 at 7 20 41 pm

Mark-Agent003 and others added some commits Jan 4, 2018

@Umcaruje

This comment has been minimized.

Member

Umcaruje commented Jan 4, 2018

I have reverted this to use full-row highlighting, and also reduced the size of the CSS because some of the changes were unnecessary.

image

@Mark-Agent003

This comment has been minimized.

Member

Mark-Agent003 commented Jan 4, 2018

@Umcaruje , any reason you didn't include selection-color?

@Umcaruje

This comment has been minimized.

Member

Umcaruje commented Jan 5, 2018

@Umcaruje , any reason you didn't include selection-color?

The font color doesn't change as is evident by my test results, so I see no point in setting it. Did you run into some issues?

@Mark-Agent003

This comment has been minimized.

Member

Mark-Agent003 commented Jan 5, 2018

Ah, I see. This has my approval. 👍

@tresf tresf merged commit 59eba30 into LMMS:stable-1.2 Jan 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Mark-Agent003 Mark-Agent003 deleted the Mark-Agent003:stable-1.2 branch Mar 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment