Linux ScreenShot feature added #319
Conversation
|
Closed this pull request accidently. Sorry about that. Please check its running fine. |
|
@bhaveshAn thanks for contributing, please add |
|
@kiok46 thanks for your review. Actually I am working on ScreenShot feature for Windows too but don't know how long it would take. So I can say that I have made the PR working from example too. If you merge this PR It will not required for me to recreate the example for screenshot in PR #324 and hence I will pull the upstream changes after deleting the example in PR of OSX. In this way, Workflow will remain same without a extra PR as you suggested.
|
plyer/facades/screenshot.py
Outdated
| @@ -0,0 +1,62 @@ | |||
| ''' | |||
| ScreenShot | |||
There was a problem hiding this comment.
According to Oxford dictionary a "screenshot" is a real word, unlike "FileChooser", therefore it'd be better to go for that. Also, I'd say it's more expected to do:
from something import Screenshot
instead of:
from something import ScreenShot
as it will be more confusing in the future. Also, it's inconsistent with the rest of your code, where you use "screenshot" as a single word.
plyer/facades/screenshot.py
Outdated
| super(ScreenShot, self).__init__() | ||
| self._file_path = file_path | ||
|
|
||
| def take_shot(self): |
There was a problem hiding this comment.
screenshot.take() seems visually more pleasant than screenshot.take_shot()
plyer/facades/temperature.py
Outdated
| @@ -1,5 +1,6 @@ | |||
| # coding=utf-8 | |||
|
|
|||
|
|
|||
| ''' | ||
| Location of the screenshot. | ||
| ''' | ||
| assert isinstance(location, (basestring, unicode)), \ |
There was a problem hiding this comment.
Haven't tried it yet, but this seems smelly to me. Have you tried it both with py2 and py3?
There was a problem hiding this comment.
It is also have been used in the audio facade from long.
There was a problem hiding this comment.
Definitely doesn't work on py3. To be more precise:
>>> basestring
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
NameError: name 'basestring' is not defined
>>> unicode
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
NameError: name 'unicode' is not defined
If there isn't some background magic going on, then I believe even the audio part is broken for py3, unfortunately.
There was a problem hiding this comment.
What say If I remove it. @KeyWeeUsr
plyer/platforms/linux/screenshot.py
Outdated
| from plyer.utils import whereis_exe | ||
|
|
||
|
|
||
| class GnomeScreenShot(ScreenShot): |
There was a problem hiding this comment.
Perhaps a reference to the options (gnome, import) might be useful in the documentation?
There was a problem hiding this comment.
It is done to give to use the feature with which-ever utitlity is present. So need is to give separate names to both implemented classes. What say @KeyWeeUsr
|
This pr specifically uses gnome for screenshot. Assuming a Userinterface is something we can not do. The way you check for ability to import Gnome then switch to using "import" cli binary is something that can be used to test different interfaces like KDE or unity or .... Please look into using XWindows directly to get the screenshot. |
|
@akshayaurora I have used command-line tools in my PRs because previously the features like Notification for linux is using tool like So, from my point of view there is no issues to use pre-installed binaries or command line tools to implement the feature, the idea that I acquire about plyer is to not having dependencies either. If I am wrong please correct me. :) |
|
Tools that are "universal & expected to be pre existing on most linux" distros are accepted if there is no obvious way around using them. In this case there is. XWindows & Xrandr extension both offer such options. |
|
@akshayaurora I browsed about it. find one such and running fine. https://wiki.debian.org/ScreenShots#Using_command_line can i use it. |
|
@akshayaurora I have added the screenshot using |
|
Opened a new pull concerning linux screenshot at #326 |
Not having dependencies either.
In a merge-able state.