Skip to content

[FEATURE] Implements a -s option to make Archey take a screenshot #75

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
Merged
merged 11 commits into from
Jun 4, 2020
Merged

[FEATURE] Implements a -s option to make Archey take a screenshot #75

merged 11 commits into from
Jun 4, 2020

Conversation

HorlogeSkynet
Copy link
Owner

@HorlogeSkynet HorlogeSkynet commented May 29, 2020 •
edited
Loading

Title should be self-explanatory 😄

Description

New CLI option/argument.
Implemented logic wants to be EAFP, handling many different cases in a "best-effort" fashion.
One may want to add its specific (omitted ?) program to the supported list (really easy).

⚠️ Important note : Sometimes it's not possible to directly specify the output filename. In such cases, the targeted directory should be honored.

⚠️ Important note (2) : This feature will be considered as a "soft-extension" of Archey, so no screenshot program has been (and will be) added to required/optional dependencies lists.

⚠️ Important note (3) : It's a first very basic implementation, it might evolve in the future.

Thanks 🙇

Reason and / or context

Well, ScreenFetch supports it, and even some other Archey forks.
So I guess such a feature was definitely missing here (?).

How has this been tested ?

Locally, with flameshot, scrot, import (ImageMagick) & gnome-screenshot.

Types of changes :

  • Bug fix (non-breaking change which fixes an issue)
  • Typo / style fix (non-breaking change which improves readability)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist :

  • [IF NEEDED] I have updated the README.md file accordingly ;
  • [IF NEEDED] I have updated the test cases (which pass) accordingly ;
  • My changes looks good ;
  • I agree that my code may be modified in the future ;
  • My code follows the code style of this project (PEP8).

@HorlogeSkynet HorlogeSkynet added the enhancement ⬆️ Implements a new feature, fixes or improves existing ones label May 29, 2020
@HorlogeSkynet HorlogeSkynet added this to the v4.8.0 milestone May 29, 2020
@HorlogeSkynet HorlogeSkynet requested a review from ingrinder May 29, 2020 14:09
@HorlogeSkynet HorlogeSkynet self-assigned this May 29, 2020
Samuel FORESTIER and others added 2 commits May 30, 2020 10:31
[SCREENSHOT] Let's accept directory from CLI and do our best to honor it
b8f1f79
@ingrinder
[SCREENSHOT] Update file-naming to more closely resemble ISO 8601
28cd846 
Also does not stop trying to take a screenshot on one program's failure,
and keeps error messages from printing until a screenshot has been taken.
@HorlogeSkynet
Copy link
Owner Author

That's very clean 👍

@ingrinder
[SCREENSHOT] Add delay before screenshot is taken.
ccdafef 
Rationale: Before, sometimes the screenshot would be taken before
Archey's output was visible in the terminal. This delay ensures that
the terminal has had the chance to display the output before we take any
screenshot.

Additionally, this commit adds a success message detailing the
screenshot program used.
@ingrinder
Copy link
Collaborator

I noticed on my system (with ImageMagick), sometimes the screenshot was being taken before the output from Archey had appeared, so I've added a fixed delay with a countdown before it's taken. Unfortunately I've tried solutions like forcing the stdout buffer to flush but there's no easy way to check when the terminal has actually rendered it, so this was the only thing I could come up with 😕

@HorlogeSkynet
Copy link
Owner Author

HorlogeSkynet commented May 31, 2020 •
edited
Loading

Other implementations also play with delay features offered by some programs (all of them ?), so I guess that makes sense.
I'm not convinced about the special print call flushing output though. I guess we may workaround the issue without it ¯_ (ツ) _/¯
EDIT : Maybe we could rather add flush=True to Output.output print call(s) instead ?

@HorlogeSkynet
Copy link
Owner Author

HorlogeSkynet commented May 31, 2020 •
edited
Loading

Woops, sorry about my previous message : I didn't get your commit well.
I'm all good with latest changes, this piece of code will evolve for sure in the near future anyway. We definitely need more feed-backs from end-users 👌

I took the liberty to remove one of your print-ed message ; I think we can keep Archey quiet if the passed command got executed well 🤷‍♂️

@ingrinder
Copy link
Collaborator

Ah yes, sorry, that sneaked in my last commit while I was figuring out what was taking the screenshot on my system -- For some reason in VS Code under XWayland, some of the environment variables are set wrong (I think it was XDG_CURRENT_DESKTOP) and the screenshot from gnome-screenshot is black... But I think this is a VS Code issue rather than anything to do with us!

As you say, I'm sure we'll be refining this implementation in future regardless 😃

@HorlogeSkynet
Copy link
Owner Author

Well that's indeed weird, good luck with the investigation 😮

ingrinder
ingrinder approved these changes Jun 3, 2020
View reviewed changes
Copy link
Collaborator

@ingrinder ingrinder left a comment

Choose a reason for hiding this comment

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

LGTM!

I'll get around to opening an upstream issue for vs-code's behaviour sooner or later, since it affects running gnome-screenshot directly from its inbuilt terminal... not our problem, if you will 😅.

@HorlogeSkynet HorlogeSkynet merged commit 73c1f68 into master Jun 4, 2020
@HorlogeSkynet HorlogeSkynet deleted the feature/screenshot branch June 4, 2020 16:23
HorlogeSkynet pushed a commit that referenced this pull request Oct 4, 2020
[SCREENSHOT] Fixes usages of unknown .rpart method
f1523ff 
> Fixes a bug introduced in #75, released in v4.8.0

[skip ci]
@HorlogeSkynet HorlogeSkynet mentioned this pull request Oct 4, 2020
Merged
4 tasks
HorlogeSkynet added a commit that referenced this pull request Oct 7, 2020
@HorlogeSkynet
[SCREENSHOT] Fixes usages of unknown .rpart method (#86)
0579f44 
> Fixes a bug introduced as long as the feature in #75, released in v4.8.0

[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ingrinder ingrinder ingrinder approved these changes

Assignees

@HorlogeSkynet HorlogeSkynet

Labels
enhancement ⬆️ Implements a new feature, fixes or improves existing ones
Projects
Archey4 - Core
Status: DONE
Milestone
v4.8.0
Development

Successfully merging this pull request may close these issues.
2 participants
@HorlogeSkynet @ingrinder