Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@danirabbit Thanks for the detailed review! I'll get working on your comments. Regarding the dialog, I was trying to reduce the amount of typing required to specify the remote URL especially if you are cloning an elementary repository. I too usually copy paste the URL from github because I use the terminal command |
Set input purpose Co-authored-by: Danielle Foré <danielle@elementary.io>
SPDX header Co-authored-by: Danielle Foré <danielle@elementary.io>
jeremypw
changed the title
danirabbit
left a comment
danirabbit
left a comment
There was a problem hiding this comment.
Just two things and then I think it's good to get in for more testing
src/Services/GitManager.vala
Outdated
| return build_path; | ||
| } | ||
|
|
||
| //TODO Make this a real async function that does not block the main loop. |
There was a problem hiding this comment.
I think this needs to be done before this is ready to merge. Otherwise Gala thinks Code has crashed while it's cloning
There was a problem hiding this comment.
Done but Gala still thinks Code has crashed because the underlying Ggit function is still synchronous and there is no obvious way of interrupting it. Once it has started it dominates the cpu. I'll try to find a solution as this affects other apps too.
There was a problem hiding this comment.
Looks like we need to use a real GLib.Thread. Async functions run in the same thread as the main loop so can block the UI.
There was a problem hiding this comment.
Now using a GLib.Thread to wrap the Ggit.clone (). Seems to work (as judged by spinner operating smoothly during cloning)
danirabbit
left a comment
danirabbit
left a comment
There was a problem hiding this comment.
Somewhere along the way we seem to have lost "activates default" on entries
| // - Can contain only letters ( a-zA-Z ), digits ( 0-9 ), underscores ( _ ), dots ( . ), or dashes ( - ). | ||
| // - Must not contain consecutive special characters. | ||
| // - Cannot end in . git or . atom . | ||
| private const string CLONE_REPOSITORY = N_("Clone Button"); |
There was a problem hiding this comment.
Sorry, left over from testing...
| public bool cloning_in_progress { | ||
| set { | ||
| if (value) { | ||
| clone_button.label = _(CLONING); | ||
| spinner.start (); | ||
|
|
||
| } else { | ||
| clone_button.label = _(CLONE_REPOSITORY); | ||
| spinner.stop (); | ||
| } | ||
|
|
||
| revealer.reveal_child = value; | ||
| } | ||
|
|
||
| get { | ||
| return revealer.reveal_child; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is definitely not right. You could add another page with a spinner, but keeping everything sensitive and changing the button label is definitely not good
There was a problem hiding this comment.
OK, the spinner at this point is a proof of principle that the cloning thread was not blocking the UI. I wasn't sure how to show it in practise. I'll show another page as you suggest.
There was a problem hiding this comment.
Now implemented simple separate page
| clone_button = new Gtk.Button.with_label (_(CLONE_REPOSITORY)); | ||
| clone_button.show (); | ||
| add_action_widget (clone_button, Gtk.ResponseType.APPLY); |
There was a problem hiding this comment.
This should probably be
| clone_button = new Gtk.Button.with_label (_(CLONE_REPOSITORY)); | |
| clone_button.show (); | |
| add_action_widget (clone_button, Gtk.ResponseType.APPLY); | |
| clone_button = add_button (_(CLONE_REPOSITORY), Gtk.ResponseType.APPLY); |
There was a problem hiding this comment.
Included in my commit
|
@danirabbit Thanks for the thorough review. Hopefully most of the essential issues are fixed now. The "cloning in progress" page is minimal but can be improved before the next release hopefully. I am still unsure whether it is correct to focus the "Cancel" button rather than the URI entry on opening. I have reinstated the "activates-default" behaviour although there is a small risk of starting to clone into the wrong folder if the intention is to clone into a non-default location. |
|
There doesn't seem to be a way of cancelling an ongoing cloning process afaict. |
|
I have a PR demonstrating use of Ggit.RemoveCallbacks to give more detailed info during cloning here #1608, but needs design work and I don't want to hold this PR. |
5 participants
Uh oh!
There was an error while loading. Please reload this page.