Skip to content

Add notification when correctly copying hash commit #1376

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

Merged
merged 12 commits into from
Oct 26, 2022

Conversation

SergioRibera
Copy link
Contributor

This Pull Request fixes/closes #1160.

It changes the following:

  • When copying the hash of a commit, it displays a confirmation dialog if it has been copied correctly

I followed the checklist:

  • I added unittests
  • I ran make check without errors
  • I tested the overall application
  • I added an appropriate item to the changelog

@alexmaco
Copy link
Contributor

alexmaco commented Oct 6, 2022

I also ran into these usability issues with copying.

Maybe it would be useful to display the text that was copied in the notification?

Additionally, copying might fail (e.g. if both xclip and xsel are missing or if gitui is being run via ssh). The notification could report that failure instead when it happens. Maybe a good place to catch that error is at the call site of copy_commit_hash in commitlist.rs?

@SergioRibera
Copy link
Contributor Author

I also ran into these usability issues with copying.

Maybe it would be useful to display the text that was copied in the notification?

Additionally, copying might fail (e.g. if both xclip and xsel are missing or if gitui is being run via ssh). The notification could report that failure instead when it happens. Maybe a good place to catch that error is at the call site of in ?copy_commit_hash``commitlist.rs

You are very right, I think it would be very useful to know what text is being copied, on the other hand I also agree with you, I think it is useful to show the error message if the copy fails, in that case I did nothing because I did not know what to do with that error, now I have an idea, so I will be making changes and will update the PR

Copy link
Collaborator

@extrawurst extrawurst left a comment

Choose a reason for hiding this comment

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

see above

@extrawurst
Copy link
Collaborator

please rebase/squash so I can merge it

@extrawurst
Copy link
Collaborator

oh and please fix clippy :)

@SergioRibera
Copy link
Contributor Author

@extrawurst That's it, I fixed all the clippy and formatting errors in the project (I think it should go in another PR but the CI was not working due to some base errors)

Cries in CI/CD *
image

If you need any changes, please let me know

@extrawurst
Copy link
Collaborator

I am so sorry I only meant the changes for stable clippy, you are right the remaining stuff should and IS in another PR already: #1390

@extrawurst
Copy link
Collaborator

#1390 is merged now, so please update with current master again

@SergioRibera SergioRibera force-pushed the feature/feedback_copying branch from 747bc80 to fefd64a Compare October 19, 2022 22:31
@SergioRibera
Copy link
Contributor Author

@extrawurst Done, now a query please, what version of rust do you use?

@extrawurst
Copy link
Collaborator

Please rebase again and resolve the conflicts so we can merge this

@extrawurst
Copy link
Collaborator

please fix the clippy errors

@SergioRibera
Copy link
Contributor Author

@extrawurst Done 😃

@extrawurst extrawurst merged commit 282e578 into gitui-org:master Oct 26, 2022
@extrawurst
Copy link
Collaborator

Thanks for sticking at this!

IndianBoy42 pushed a commit to IndianBoy42/gitui that referenced this pull request Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide some feedback after copying commit hash
3 participants