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.

Sign up for GitHub

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

Jump to bottom

rambox: allow user to disable tooltips #32741

Merged
merged 1 commit into from Dec 16, 2017
Merged

rambox: allow user to disable tooltips #32741

merged 1 commit into from Dec 16, 2017

Conversation

ghost
Copy link

@ghost ghost commented Dec 16, 2017

Motivation for this change

Rambox now annoys user with donate banner on every launch no matter which button is used to close tooltip, so add an option to disable tooltips, since developer doesn't want to provide tooltip disabling option.

screenshot_20171216_195328

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

orivej
orivej approved these changes Dec 16, 2017
View reviewed changes
Copy link
Contributor

@orivej orivej left a comment •
edited

Choose a reason for hiding this comment

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

/cc @yegortimoshenko

Initially I though that disableDonationTooltip would be a better name, but it seems that the developer may choose to show any tooltip (at https://tooltip.io/), so this option indeed disables the tooltip player rather than just donation tooltips.

@lukateras
Copy link
Member

lukateras commented Dec 16, 2017 •
edited

Given circumstances, I think this should be true by default until upstream fixes the issue. I don't
think that's intended, but it's broken. (I don't have anything against donation banners per se.)

Also I don't like the remote aspect of it.

[...] since developer doesn't want to provide tooltip disabling option.

Also upstream closed my request to provide an option to disable updates (with a partial patch) seemingly without much consideration...

Initially I though that disableDonationTooltip would be a better name, but it seems that the developer may choose to show any tooltip (at https://tooltip.io/), so this option indeed disables the tooltip player rather than just donation tooltips.

Yes, it's the same tooltip that the developer uses to notify about Rambox updates, if I remember correctly, which is problematic by itself since we handle package updates ourselves.

disableTooltipIo, disableTooltip, disableTooltips, are all good names.

@ghost
Copy link
Author

ghost commented Dec 16, 2017

@yegortimoshenko Switched to true, thanks. That's not the first time we have such issue https://github.com/saenzramiro/rambox/issues/414

Also I don't like the remote aspect of it.

Yes, that's bad.

@ghost
Copy link
Author

ghost commented Dec 16, 2017

Also https://github.com/saenzramiro/rambox/issues/1030#issuecomment-335478065

Blocking Tooltip.io is not a good option because sometimes we communicate important with Tooltip.

Never seen any important tooltip from them for years.

@orivej
Copy link
Contributor

orivej commented Dec 16, 2017

It seems unfair to deny the developer the chance to display their popup at least once to the new users, especially the donation popup. In general I have nothing against apps that display one popup on each startup (which was typical of shareware apps), and I would not want to disable this popup by default even if upstream admitted that they want to display it to every user on every startup. The two options I see are (1) disable the popup with a comment "enable with the update to 0.5.14 to reevaluate" or "enable when upstream closes issue number X", or (2) provide the version without popup under a different attribute, e.g. rambox-nopopup and a comment with an explanation or a link to this PR.

@ghost
Copy link
Author

ghost commented Dec 16, 2017

@orivej I've put it back. There is not much reason to complicate things.
Better to try build Franz (Rambox clone) from sources too, since that became open source recently.
BTW I started to have another issue few days ago with Rambox, which is hard to debug - just the white screen after some usage
screenshot_20171216_195343

@lukateras
Copy link
Member

lukateras commented Dec 16, 2017 •
edited

It seems unfair to deny the developer the chance to display their popup at least once to the new users, especially the donation popup.

I agree.

In general I have nothing against apps that display one popup on each startup (which was typical of shareware apps), and I would not want to disable this popup by default even if upstream admitted that they want to display it to every user on every startup.

The problem here is that there is a way to disable popups in a shareware program: pay for it. In this case even users who have paid still get that popup on each launch. I don't think that's fair, because the expectation is that it will go away.

Also, using a service to show tooltips is inherently problematic: it means it can be remotely controlled at any time, and it might show information that we don't necessarily want to show to our users, like new version notifications.

Developer has already closed the issue with this request (saenzramiro/rambox#414), saying he'd look for an alternative. I agree that it might be morally questionable to disable tooltip.io, and I agree with @gnidorah that it would be easier to just leave it as it is by default.

Also, I don't use Rambox, I've only helped to package it, so I'm not affected either way :-)

@orivej orivej merged commit f6dd6a4 into NixOS:master Dec 16, 2017
@orivej
Copy link
Contributor

orivej commented Dec 16, 2017

BTW I started to have another issue few days ago with Rambox, which is hard to debug - just the white screen after some usage

It is easy to reproduce by focusing and defocusing Rambox window, occasionally resizing it. The window becomes white when this is printed to the console:

[5268:1216/203518.057807:FATAL:partition_alloc.cc(934)] Check failed: page->num_allocated_slots != -1.

@lukateras
Copy link
Member

https://chromium.googlesource.com/chromium/src.git/+/62.0.3178.1/base/allocator/partition_allocator/partition_alloc.cc#934

Electron was recently updated (680f3ad), that's probably what's causing the issue.

@ghost ghost deleted the rambox branch December 17, 2017 03:13
@ghost
Copy link
Author

ghost commented Dec 17, 2017

@orivej @yegortimoshenko Ah, thanks much!

@lukateras lukateras mentioned this pull request Dec 17, 2017
Merged
8 tasks
orivej pushed a commit to lukateras/nixpkgs that referenced this pull request Dec 18, 2017
@lukateras @orivej
electron: 1.8.1 -> 1.7.9
a9d212f 
1.7.9 is the latest stable, and 1.8.1 is beta and it draws `rambox` as a white
rectangle (after a few manipulations with the window such as hiding, showing,
resizing): NixOS#32741 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@lukateras @orivej