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

Reduce dependence on D-Bus #2003

Merged
merged 48 commits into from
Dec 8, 2021
Merged

Conversation

veracioux
Copy link
Contributor

@veracioux veracioux commented Oct 23, 2021

Closes #1292.
Closes #1480.
Closes #1579.
Closes #1716.
As a side effect: Closes #1771.
May fix #1872.
May fix #1673.
My gut instinct says that this could be involved with #1246 somehow.

NOTE: I tested nothing on a Mac

Terminology

  • Daemon - The instance of flameshot that runs in the background
  • Daemon hosting - The act of running the daemon in order to support some persistent phenomenon (pinned screenshot, clipboard, etc.)

Things you should know:

  • Flameshot does not support CLI arguments on Windows (and possibly Mac, can't test 😢)
  • When you create a pin, the daemon will host it. If it is not running
  • The clipboard gets lost once flameshot quits on X11. Because of this, when any subcommand copies something to the clipboard, the daemon will be asked to host it. Will have to change this behavior for Windows and Mac.
  • If there is no need to host something, the daemon will quit automatically.
    I.e. when all pinned widgets are closed, or when the clipboard is changed by another application.

Changes and TODO (there will be more):

  • Pin and clipboard are hosted by the daemon
  • flameshot screen does not rely on dbus (except for hosting clipboard and pins)
  • flameshot full does not rely on dbus (...)
  • flameshot gui does not rely on dbus (...)
  • Do not show tray icon in non-dbus commands
  • Make flameshot launcher independent from dbus
  • Make flameshot config independent from dbus
  • flameshot launcher crashes when launching GUI
  • Look into addressing working only first time in sway. #1672 in this
  • "Copy file path after save" does not work properly
  • flameshot gui mutual exclusivity
  • Add option autoCloseIdleDaemon, unix-only, default: false
  • Tray icon should behave as it did before
  • Add the new config options to the GUI
  • Edit test script for MacOS (Jeremy) (Reduce dependence on D-Bus #2003 (comment))

TODO (implementation details):

  • Reconcile Controller and FlameshotDaemon - make a clear distinction between them
  • Remove unnecessary arguments in Controller::captureTaken(Failed)
  • Clean up obsolete code

TODO (when merging)

Testing:

  • Before testing this PR, make sure the previous daemon version is killed
  • Everything should work as it did before this PR
  • If you have a Windows and/or Mac machine, please test thoroughly. I don't have
    a Mac machine.
  • Run the scripts in the tests directory inside the repository root. These will test most of the CLI features, but not all!
  • Abuse the screen, full and gui subcommands with various options and verify that the daemon is not launched, except when it needs to host the clipboard and/or pinned screenshots. You can use the script tests/action_options.sh
  • Check if flameshot gui works correctly with various options and various types of manual interaction
  • Test if stuff is copied to the clipboard correctly, e.g. paste into GitHub/gimp/etc and/or run xclip -o -t TARGETS -selection clip
  • If the daemon is not needed, it should quit automatically (on Linux). That is when all pinned screenshot windows are closed and the clipboard gets changed externally. On Windows and Mac, the daemon should keep running.
  • Please test the heck out of daemon auto-launching, because this never worked
    on my system for some reason. In other words: make sure to kill the daemon before doing the other tests.

To be discussed

  • Do we allow multiple instances of flameshot gui to run? A benefit of this is the ability to take screenshots of flameshot itself.
  • Add a config option to keep daemon running even if not needed??
  • What should we do with the system tray if the daemon is running merely to host pins, clipboard, etc?

Signed-off-by: Haris Gušić <[email protected]>
We need to wait until the upload widget (or similar widgets) have
finished before exiting. This must be done using a signal. The problem
is that CaptureRequest can't be guaranteed to survive until the widget
has finished what it's doing.

Signed-off-by: Haris Gušić <[email protected]>
Signed-off-by: Haris Gušić <[email protected]>
Signed-off-by: Haris Gušić <[email protected]>
The messages were caused by the color wheel.

Signed-off-by: Haris Gušić <[email protected]>
Signed-off-by: Haris Gušić <[email protected]>
Signed-off-by: Haris Gušić <[email protected]>
@veracioux
Copy link
Contributor Author

I have tested on Windows now. Everything should work as it did before this PR. But the MVP is someone who can test this on MacOS, especially someone who can make CLI work there (shouldn't be too difficult, just remove a few preprocessors probably).

I tried to make CLI parsing work on Windows. It turns out you can't even print to stdout if you don't make flameshot a console application. Even if we enabled the parsing, we would still need some form of IPC to make all the features work. Not worth it! And I don't have the patience to suffer through Windows, Visual Studio & co any more than the bare minimum necessary. Sorry.

@veracioux
Copy link
Contributor Author

@mmahmoudian As my most reliable tester 😉, I'd like you to test this when you find the time.

@borgmanJeremy
Copy link
Contributor

I can commit to testing on Mac, ive got a reliable VM set up.

src/main.cpp Outdated Show resolved Hide resolved
@borgmanJeremy
Copy link
Contributor

borgmanJeremy commented Oct 24, 2021

I havent totally finished the review, but it nominally works on my Mac VM. I also posted a comment of the macro that needs changed to enable to enable CLI parsing on Mac. I didnt test every combination of arguments but it nominally worked
Mac
.

Signed-off-by: Haris Gušić <[email protected]>
@borgmanJeremy
Copy link
Contributor

The test script also seems to nominally work, but it will need some tweaks for MacOs in the future. Namely notify-send and display do not exist on Mac.

@veracioux
Copy link
Contributor Author

@borgmanJeremy Could you tweak the script and send it to me, or submit it as a PR to veracioux:die_dbus?

@veracioux
Copy link
Contributor Author

@borgmanJeremy Replying to #2054 (comment)

It should be now. The only chore that's left to do is to customize the test script for MacOS, but you can do that in a separate PR.

Sorry it took me so long. There was a problem with QSettings when multiple flameshot gui instances are running causing an endless loop of file accesses. Took me a long time to debug that.

Right now I'm thinking that we can get rid of DBus altogether. But I'd like to merge this ASAP and leave that for a future PR.

@borgmanJeremy
Copy link
Contributor

Ok awesome I'll check it out in a day or so.

I don't think we can ever get rid of dbus entirely as I am not aware of another way to take a screenshot on wayland .

@borgmanJeremy
Copy link
Contributor

I made the adjustment to enable MacOS CLI parsing but it seg faults. Need to troubleshoot
image

@borgmanJeremy
Copy link
Contributor

It seems to be working today, must have been an OS issue. I have been testing manually but will work on porting the script now. The only issue I have identified so far is the --clipboard option is not working.

@borgmanJeremy
Copy link
Contributor

borgmanJeremy commented Dec 8, 2021

The segfault was fixed with this commit: 58e37b0

I think all thats left is to fix the test script. @veracioux is there a reason you send a notification instead of just echoing to the command line?

Okay @veracioux the notifications are fixed on the macos_dbus branch. Github would not let me submit this as a PR to your fork for some reason. The only remaining issue is 'display' does not work on Macos but I would really like to get this merged ASAP before more merge conflicts happen. I say lets merge this week.

@veracioux
Copy link
Contributor Author

@borgmanJeremy Thanks, I'll try to merge it tonight.

@veracioux veracioux merged commit 233c765 into flameshot-org:master Dec 8, 2021
razzolini added a commit to razzolini/dotfiles that referenced this pull request Feb 25, 2022
This reverts commit f79ebad.

Apparently, the recommendation to start xmonad with dbus-launch may have
been obsolete, because nowadays (on Arch Linux) dbus is started by
systemd (see https://bugs.archlinux.org/task/60179, specifically
"Additional comments about closing").

Also, flameshot recently updated to v11.0.0, which ― if I understood the
changelog correctly ― should no longer require dbus for CLI usage (see
https://github.com/flameshot-org/flameshot/releases/tag/v11.0.0 and
flameshot-org/flameshot#2003).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment