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

feat: Change cursors over clickable/scrollable areas #727

Merged
merged 9 commits into from Sep 17, 2017

Conversation

NBonaparte
Copy link
Member

Implements changing cursors when a clickable/scrollable area is hovered over. This can be configured in the bar settings using cursor-click and cursor-scroll, where the supported cursors (other than the default) are pointer and ns-resize (using the CSS values).

I currently have the xcb-cursor dependency as optional because the polybar-git AUR package would need to be updated.

I might implement disabling this feature per-module in the future as requested in #721, but maybe in a later PR as it will be fairly complicated.

Closes #721.

@NBonaparte NBonaparte changed the title Change cursor feat: Change cursors over clickable/scrollable areas Sep 6, 2017
@patrick96
Copy link
Member

I'll take a look at this tomorrow

Copy link
Member

@patrick96 patrick96 left a comment

Choose a reason for hiding this comment

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

This looks great. There are some minor things, I think should be addressed.
This works on my system, I'm currently setting up a testing VM with multiple WMs, I'll test it over there too once I'm done.


POLYBAR_NS

namespace cursor_util {
Copy link
Member

Choose a reason for hiding this comment

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

Do I understand this right that there are three types of cursors pointer, ns-resize and default and these vectors are just a list of fallback names if the xcb doesn't recognize a certain cursor name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I have chosen three relevant cursors to be used (we could add more but IMO those are the ones which are most likely to be used). Their names are not standardized so we have to cycle through them until we find one that exists in the current theme.


namespace cursor_util {
static const vector<string> pointer_names {"pointing_hand", "pointer", "hand", "hand1", "hand2", "e29285e634086352946a0e7090d73106", "9d800788f1b08800ae810202380a0822"};
static const vector<string> arrow_names {"left_ptr", "arrow", "dnd-none", "op_left_arrow"};
Copy link
Member

Choose a reason for hiding this comment

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

For clarity we should probably name this default_names

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough.

@@ -126,6 +130,8 @@ bar::bar(connection& conn, signal_emitter& emitter, const config& config, const
m_opts.dimvalue = m_conf.get(bs, "dim-value", 1.0);
m_opts.dimvalue = math_util::cap(m_opts.dimvalue, 0.0, 1.0);

m_opts.cursor_click = m_conf.get(bs, "cursor-click", ""s);
m_opts.cursor_scroll = m_conf.get(bs, "cursor-scroll", ""s);
Copy link
Member

Choose a reason for hiding this comment

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

I think having pointer and ns-resize as default values would be a good idea

Copy link
Member

Choose a reason for hiding this comment

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

Also it would be useful to the user if polybar showed a warning, if the config uses an invalid cursor name.
We could maybe have a map that maps cursor names to name vectors. Like this we have all valid cursor names as well as their xcb names in one spot and set_cursor in cursor.cpp could just access the map instead of using a long if-elseif chain. This would also make it easier to, in the future, add new cursor names.

@patrick96
Copy link
Member

Do you think xcb-util-cursor should stay an optional dependency?
I think we should make it a normal dependency in all the package scripts (AUR, XBPS etc) but leave all the #if WITH_XCURSOR checks, so that it potentially could be built without
In any case we would need to change the PKGBUILD for polybar-git (and for polybar once we release a new version).

@NBonaparte
Copy link
Member Author

I've made it optional for now because currently we don't have write access to the packages. Once we are able to change them, I'll probably make it a normal dependency.

@patrick96
Copy link
Member

I think this PR should then add xcb-util-cursor to the polybar-git PKGBUILD dependencies and it will become a normal dependency as soon as jaagr gets the time to push the updated PKGBUILD to the AUR (or one of us gets push access).
For the regular PKGBUILD we just need to keep in mind to add it, once we prepare a new release

@NBonaparte NBonaparte added this to the 3.0.6 milestone Sep 10, 2017
@NBonaparte NBonaparte force-pushed the change-cursor branch 2 times, most recently from 34b3a17 to 9b9e8c5 Compare September 13, 2017 02:37
bool found_scroll = false;
const auto find_click_area = [&](const action& action) {
if (!m_opts.cursor_click.empty() &&
(action.button == mousebtn::LEFT || action.button == mousebtn::MIDDLE || action.button == mousebtn::RIGHT)) {
Copy link
Member

Choose a reason for hiding this comment

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

Double clicks probably should also show the click cursor

@patrick96
Copy link
Member

I have also found an issue when switching between areas that should show the cursor and scroll pointers.
Consider the following modules next to each other:

[module/scroll]
type = custom/text
content = Scroll me %{A1:notify-send "Click over scroll":}Click ME%{A}

scroll-up = notify-send "Scroll Up"

[module/click]
type = custom/text
content = Click me %{A4:notify-send "Scroll not over click":}scroll here%{A}

click-middle = notify-send "Click-middle"

The "Click ME" area should show the click cursor pointer, if approached from the left, the scroll cursor is shown and if approached from the right, the click cursor is shown.
Here is a gif of the phenomenon:
cursor_bleed

@NBonaparte
Copy link
Member Author

Should be fixed now, please check.

Copy link
Member

@patrick96 patrick96 left a comment

Choose a reason for hiding this comment

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

Can you also add the two new options to the sample configuration with the values pointer and ns-resize.
I think after that we can merge this

@patrick96 patrick96 merged commit 4663d01 into polybar:master Sep 17, 2017
@NBonaparte NBonaparte deleted the change-cursor branch September 25, 2017 18:59
patrick96 added a commit that referenced this pull request Dec 3, 2017
Breaking Changes:

* Date module no longer supports non-padded specifiers (i.e. `%-d`) and potentially other specifiers, see #792
  - Check http://en.cppreference.com/w/cpp/io/manip/put_time to see supported specifiers
* Setting background color to `background-0` with gradients (refer to https://github.com/jaagr/polybar/wiki/Known-Issues)

Changelog:

Features:
* Feat(mpd): State-specific formats (`format-playing`, `format-paused`, `format-stopped`) (#567), see #524 
* Feat(ipc): Visibility commands (show, hide, toggle, restart, quit) (b6c5563)
* Feat(shell): Bash completion (#588)
* Feat(menu): `expand-right` option (#658), see #655
* Feat(temperature): hwmon sysfs support (#688), see #404 
* Feat(cursor): Change cursors over clickable/scrollable areas (#727), see #721  
* Feat(temperature): Fahrenheit and Celsius tokens (#804)
* Feat(mpd): Use mpd name tag or URI as fallback for title-less tracks (#823), see #815 

Fixes:
* Fix(i3): Clicking workspaces without index (#521), see #520 
* Fix(parser): Prefix options overriding format options (#729), see #544
* Fix(parser): Overline tags (eebf105)
* Fix(process_util): Prefix shell environment variable (`$POLYBAR_SHELL`) (86ff947), see #566 
* Fix(parser): `%{R}` tag (reverse colors) (0bd8f1f), see #585 
* Fix(renderer): Center block position with tray (389bae2 & #673), see #551 & #672 
* Fix(xworkpaces): Active workspace with XMonad (#587), see #411 & #535 
* Fix(config): Expand tilde, environment variable (d3b0670 & #724), see #603 & #719 
* Fix(build): Remove curlbuild.h (#648), see #647 
* Fix(renderer): Off by one error for actions (#663), see #661 
* Fix(gcc): GCC 7.1 ([jaagr/xpp/#6](polybar/xpp#6))
* Fix(fs): Use `bytes_available` for `percentage_used` (138f5fa), see #710
* Fix(fs): Use `f_frsize` for calculations (a682d2a)
* Fix(date): Remove date string length limitation (#745), see #754 
* Fix(renderer): Nested actions (#772), see #760 and #758
* Fix(i3): Check and warn if current workspace not found (#826), see #824 
* Fix(github): Prevent module disappearing with no connection (#811), see #810 
* Fix(renderer): Module gradients (#831), see #759 
* Fix(build): Update deprecated jsoncpp Reader
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants