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

Add delete tags functionality #9796

Merged
merged 12 commits into from
May 27, 2020
Merged

Add delete tags functionality #9796

merged 12 commits into from
May 27, 2020

Conversation

rafeca
Copy link
Contributor

@rafeca rafeca commented May 14, 2020

Closes #9770

This PR is based on #9828

Description

This PR adds the functionality to delete tags that haven't been pushed yet from GitHub Desktop.

In order to do so, a new context menu item has been added to commits that have unpushed tags. If the commit has one single unpushed tag, the menu iten will show a text like "Delete tag v1.0.0", but if there are more than one unpushed tags, the menu item will display a "Delete tags..." text and will show the tags that can get deleted via a submenu.

In order to implement that, the context menu behaviour has been changed to support submenus in context menus (see commit 55443cd).

@tierninho in order to test this PR you'll have to run yarn build:dev beforehand, since it modifies code from the main process.

Screenshots

demo

Release notes

Notes: [Added] Allow to delete tags that have not been pushed to the remote repository

@rafeca rafeca requested review from niik and outofambit May 14, 2020 16:51
@tierninho
Copy link
Contributor

tierninho commented May 14, 2020

This just randomly popped up in the console of this build (a462040), however I do not see delete tags functionality despite running the yarn build:dev:

install.ts:23 `git -c credential.helper= push origin sdfsdf:sdfsdf --follow-tags --progress` exited with an unexpected code: 1.
stderr:
error: src refspec sdfsdf matches more than one
error: failed to push some refs to 'https://github.com/tierninho/Blah.git'

@tierninho tierninho self-requested a review May 14, 2020 21:52
tierninho
tierninho previously approved these changes May 14, 2020
Copy link
Contributor

@tierninho tierninho left a comment

Choose a reason for hiding this comment

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

  • Functionally everything works great for OSX, however there is a 2-3 sec lag in getting the tag to be recognized by Desktop. For example, if I create a tag and immediately try to delete it, there is a lag which prohibits me from doing soon right away. Is this due to the dev env?

  • Secondly, perhaps we should always show the Delete tag menu option, but just disable it if the logic does not apply? It would be consistent with the View on Github option here:
    Screen Shot 2020-05-14 at 11 50 46 AM

    There is an edge case if we opt to go this route. If a pushed commit with an existing tag gets a new second tag, we would have to enable Delete tag option, but in the secondary context menu, only show the new tag as enabled.

--

In Windows after running yarn build:dev I am getting this error and I cannot successfully spin up:

Screen Shot 2020-05-14 at 12 03 08 PM

@ruggi99
Copy link
Contributor

ruggi99 commented May 14, 2020

@tierninho see #9396

@rafeca
Copy link
Contributor Author

rafeca commented May 15, 2020

Secondly, perhaps we should always show the Delete tag menu option, but just disable it if the logic does not apply? It would be consistent with the View on Github option here

Thanks for pointing to the "View on GitHub" option, I agree we should be consistent here (I remember @ampinsk also mentioning that to me for the submenu, so we seem to all agree), so I'll change this.

There is an edge case if we opt to go this route. If a pushed commit with an existing tag gets a new second tag, we would have to enable Delete tag option, but in the secondary context menu, only show the new tag as enabled.

Yes I think that this should be fine, do you think this can cause problems/confussion?

Functionally everything works great for OSX, however there is a 2-3 sec lag in getting the tag to be recognized by Desktop. For example, if I create a tag and immediately try to delete it, there is a lag which prohibits me from doing soon right away. Is this due to the dev env?

Yup, this delay is caused by the network request that is done to check for unpushed tags. This delay will probably go away once #9800 is fixed.

@rafeca rafeca added this to Review In Progress in Desktop 2.5.x releases via automation May 15, 2020
@rafeca rafeca changed the base branch from development to do-not-fetch-unpushed-tags May 19, 2020 17:49
Copy link
Member

@niik niik left a comment

Choose a reason for hiding this comment

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

Looking good @rafeca, a few nitpicks from me and one theoretical concern, happy to discuss that when we sync up later today

app/src/ui/delete-tag/delete-tag-dialog.tsx Outdated Show resolved Hide resolved
app/src/ui/history/commit-list-item.tsx Outdated Show resolved Hide resolved
app/src/ui/main-process-proxy.ts Outdated Show resolved Hide resolved
app/src/ui/main-process-proxy.ts Outdated Show resolved Hide resolved
@niik niik self-assigned this May 22, 2020
@tierninho
Copy link
Contributor

I opened this PR in Windows, but every time I try to create or delete a tag it crashes with this error:

TypeError: indices is not iterable
    at EventEmitter.<anonymous> (http://localhost:3000/build/renderer.js:125188:29)
    at EventEmitter.emit (events.js:203:13)
    at Object.onMessage (electron/js2c/renderer_init.js:2399:16)

Version: 2.5.1-beta1
OS: Windows 10.0.18362

@niik
Copy link
Member

niik commented May 23, 2020

@tierninho You’ll have to rebuild the main process for this to work. yarn build:dev before your yarn start should do the trick but let me know if it doesn’t

@rafeca rafeca force-pushed the do-not-fetch-unpushed-tags branch from 6423e9e to 22016b6 Compare May 25, 2020 11:21
@rafeca rafeca requested a review from niik May 25, 2020 11:25
Base automatically changed from do-not-fetch-unpushed-tags to development May 25, 2020 13:22
Copy link
Member

@niik niik left a comment

Choose a reason for hiding this comment

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

Just a tiny question remaining. I'm fine with not addressing it but it felt like such a small change

app/src/lib/stores/app-store.ts Outdated Show resolved Hide resolved
@rafeca rafeca requested a review from niik May 26, 2020 10:52
@tierninho tierninho self-requested a review May 27, 2020 00:18
Copy link
Contributor

@tierninho tierninho left a comment

Choose a reason for hiding this comment

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

Confirming that functionality works great on both OSX/Win however I spotted this edge case:

  • User can delete a tag while commits are pushed.
    • Add a tag and push
    • While the tag is being right-click to delete the tag
    • Wait until push finishes in background (Delete modal should still be open).
    • Notice that at this point, a user should not be able to delete a tag
    • Click Delete and it is removed
    • Click Fetch to bring it back

Also, a suggestion:

  • Enabling Delete tags... in this instance was confusing as I thought my menu was broken.
    Screen Shot 2020-05-26 at 2 29 52 PM
    • Should we disable the Delete tags... menu option if all tags cannot be deleted? It would match a single tag menu item like so:
      Screen Shot 2020-05-26 at 2 30 15 PM
      and we would only make the menu intact when a fresh tag is added like so:
      Screen Shot 2020-05-26 at 2 31 43 PM

Copy link
Member

@niik niik left a comment

Choose a reason for hiding this comment

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

Awesome, for the sake of getting us set up for a beta I'm going to merge this as-is and leave the suggestions @tierninho made to be considered separately

@niik niik merged commit a1df6f7 into development May 27, 2020
Desktop 2.5.x releases automation moved this from Review In Progress to PRs For Next Beta May 27, 2020
@niik niik deleted the delete-tags branch May 27, 2020 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Desktop 2.5.x releases
PRs For Next Beta
Development

Successfully merging this pull request may close these issues.

Delete tags that haven't yet been pushed
4 participants