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

Rewrite overwrite prompt for link case #2050

Merged
merged 10 commits into from
Feb 23, 2023

Conversation

mcclurgm
Copy link
Contributor

@mcclurgm mcclurgm commented Jun 10, 2022

Follow-up for #1989. Split into a separate PR so the functionality of the main one could be merged.

I think this is a more understandable wording for this case. I am open to feedback on this though.

Note that it fails to compile, seemingly because the result of printf is owned, but primary and secondary are unowned. I'm not quite sure how to fix this.

I also added a doc comment for create_overwrite_dialog while here.

@mcclurgm mcclurgm requested a review from a team June 10, 2022 22:50
filechooser-portal/Main.vala Outdated Show resolved Hide resolved
filechooser-portal/Main.vala Outdated Show resolved Hide resolved
filechooser-portal/Main.vala Outdated Show resolved Hide resolved
filechooser-portal/Main.vala Outdated Show resolved Hide resolved
@jeremypw
Copy link
Collaborator

@mcclurgm I have tried to fix this consistent with @Marukesu 's suggestions (although I have reworked the string handling a little differently). It now compiles and seems to work with the builtin portal tester and also the Flatpak version of Code. I have little experience of documenting functions - I have tried to follow the format in existing projects but there may be some tweaking required.

@jeremypw jeremypw requested a review from Marukesu February 19, 2023 18:41
@jeremypw
Copy link
Collaborator

jeremypw commented Feb 19, 2023

@Marukesu Could cast another eye over this? Particularly the method documentation I added. The CI failure is not related to this PR - it seems to be happening to all PRs merged with the current master but I haven't determined the reason yet (it doesn't happen locally so something to do with the container?)

Copy link
Contributor

@Marukesu Marukesu left a comment

Choose a reason for hiding this comment

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

I think code is good. added suggestions about commentary style and expanding the descriptions.

filechooser-portal/Main.vala Outdated Show resolved Hide resolved
filechooser-portal/Main.vala Outdated Show resolved Hide resolved
filechooser-portal/Main.vala Outdated Show resolved Hide resolved
filechooser-portal/Main.vala Outdated Show resolved Hide resolved
filechooser-portal/Main.vala Outdated Show resolved Hide resolved
Jeremy Wootten and others added 2 commits February 22, 2023 10:01
Co-authored-by: Gustavo Marques <pushstarttocontinue@outlook.com>
@jeremypw jeremypw enabled auto-merge (squash) February 22, 2023 10:42
Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

I think this can be merged now.

@jeremypw jeremypw mentioned this pull request Feb 22, 2023
6 tasks
@jeremypw jeremypw merged commit a10e4e8 into elementary:master Feb 23, 2023
@mcclurgm mcclurgm deleted the overwrite-link-copy branch October 17, 2023 02:08
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.

3 participants