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

Can't delete inline comment after last upgrade #29920

Closed
joker-eph opened this issue Sep 30, 2016 · 19 comments
Closed

Can't delete inline comment after last upgrade #29920

joker-eph opened this issue Sep 30, 2016 · 19 comments
Labels
bugzilla Issues migrated from bugzilla confirmed Verified by a second party obsolete Issues with old (unsupported) versions of LLVM

Comments

@joker-eph
Copy link
Collaborator

Bugzilla Link 30572
Version unspecified
OS All
CC @AaronBallman,@jlebar,@MatzeB

Extended Description

I can now only edit, but not delete comment. When I click on the trash it grays and stays like that forever.

@joker-eph
Copy link
Collaborator Author

I have to first edit the comment for this to happen apparently.

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 8, 2016

Hi Mehdi,

Do you mean unposted inline comments or comments to the patch?

I've tried deleting unposted inline comments by clicking the "trash" button, which always worked. I tested with Chrome browser; which browser and version are you using?

For comments to the patch, there is no "delete" button. The comment is deleted when you delete all the text, although the comment preview might not be updated immediately after you delete the text.

  • Eric

@joker-eph
Copy link
Collaborator Author

I'm talking about unposted inline comments.

Have you tried first editing and saving it before trying to delete it?

@AaronBallman
Copy link
Collaborator

FWIW, I ran into this same issue. I tried editing the comment to be empty and saving, but it was not allowed. I tried editing the comment to not be empty and saving, then deleting. The save worked, but the delete still failed. Eventually, I edited the comment to complain about the phab bug and hit submit.

I have not had this issue with all comments I try to delete. It's only happened to me one time, and I'm not certain what steps I took to get me into the weird state.

@MatzeB
Copy link
Contributor

MatzeB commented Nov 11, 2016

The same just happened to me. I can reliably reproduce it by:

  • Add inline comment to some line in the source, save it.
  • Edit the comment, slightly changing the text.
  • Press the remove button, they comment becomes greyed out but does not disappear and will be back once you reload the page.

@MatzeB
Copy link
Contributor

MatzeB commented Nov 12, 2016

Moved to Website/Phabricator (see #29736 )

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 12, 2016

Thanks Matthias! I can finally reproduce this. I'll check if this is caused by my local Phab modification. Otherwise, I'll check if this is fixed in Phab upstream or report the issue there.

@jlebar
Copy link
Member

jlebar commented Jan 13, 2017

Friendly ping?

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 13, 2017

Sorry that I lost track of this issue.

This only happens in the production instance but not the testing instance (with the same revision and local changes), so I couldn't report the issue to Phabricator upstream with a reproducer. I tried hunting the bug myself but failed (Unfortunately, I am not a phabricator or php expert :(

I can try syncing to upstream head to see if this can be magically fixed.

@jlebar
Copy link
Member

jlebar commented Jan 13, 2017

This only happens in the production instance but not the testing instance

Well that's a bummer. Maybe it's load/timing related.

I can try syncing to upstream head to see if this can be magically fixed.

That would certainly be appreciated. If nothing else maybe it will fix some of the other outstanding issues.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 16, 2017

Looks like the recent phabricator upgrade resolved this issue.

Could you please check if this is fixed for you? Thanks!

@jlebar
Copy link
Member

jlebar commented Jan 18, 2017

I can't speak to the comment-deletion bug (I was never able to reproduce on-command, so if you were, I believe you when you say that it's better now).

Unfortunately now I cannot select text inside of inline phab comments. (Chrome 55, Ubuntu.)

@MatzeB
Copy link
Contributor

MatzeB commented Jan 18, 2017

The comment deletion bug is fixed.
@​jlebar: I am not exactly sure what you mean, but selecting text in the inline comment box within the sourcecode and in the same text in that timeline view above the code with the mouse for copy&pasting work for me on Safari and chrome 55.0.2883.95 on macOS.

@jlebar
Copy link
Member

jlebar commented Jan 18, 2017

@​jlebar: I am not exactly sure what you mean, but selecting text in the
inline comment box within the sourcecode and in the same text in that
timeline view above the code with the mouse for copy&pasting work for me on
Safari and chrome 55.0.2883.95 on macOS.

Yes, I think we're talking about the same thing.

STR for me: Load https://reviews.llvm.org/D28794, ctrl+f, search for "is there a reason". There are two instances of this phrase in the page.

The first instance appears outside of the patch body. I can highlight this fine. The second instance appears inside of the patch body. I am unable to highlight this text using my mouse cursor.

@MatzeB
Copy link
Contributor

MatzeB commented Jan 18, 2017

@​jlebar: I am not exactly sure what you mean, but selecting text in the
inline comment box within the sourcecode and in the same text in that
timeline view above the code with the mouse for copy&pasting work for me on
Safari and chrome 55.0.2883.95 on macOS.

Yes, I think we're talking about the same thing.

STR for me: Load https://reviews.llvm.org/D28794, ctrl+f, search for "is
there a reason". There are two instances of this phrase in the page.

The first instance appears outside of the patch body. I can highlight this
fine. The second instance appears inside of the patch body. I am unable to
highlight this text using my mouse cursor.

Yes this works for me on macOS.

@AaronBallman
Copy link
Collaborator

@​jlebar: I am not exactly sure what you mean, but selecting text in the
inline comment box within the sourcecode and in the same text in that
timeline view above the code with the mouse for copy&pasting work for me on
Safari and chrome 55.0.2883.95 on macOS.

Yes, I think we're talking about the same thing.

STR for me: Load https://reviews.llvm.org/D28794, ctrl+f, search for "is
there a reason". There are two instances of this phrase in the page.

The first instance appears outside of the patch body. I can highlight this
fine. The second instance appears inside of the patch body. I am unable to
highlight this text using my mouse cursor.

Yes this works for me on macOS.

Also works for me on Windows 10 with Chrome 55.0.2883.87 m.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
@llvmbot llvmbot added the confirmed Verified by a second party label Jan 26, 2022
@JOE1994
Copy link
Member

JOE1994 commented Mar 13, 2024

Closing this as we're no longer using https://reviews.llvm.org for patch reviews.

@JOE1994 JOE1994 closed this as not planned Won't fix, can't repro, duplicate, stale Mar 13, 2024
@Endilll Endilll added obsolete Issues with old (unsupported) versions of LLVM and removed website labels Mar 13, 2024
@Endilll
Copy link
Contributor

Endilll commented Mar 13, 2024

@JOE1994 Can you use obsolete label for such issues?

@JOE1994
Copy link
Member

JOE1994 commented Mar 13, 2024

@JOE1994 Can you use obsolete label for such issues?

I'll update such tickets that I've closed so far with the obsolete label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla confirmed Verified by a second party obsolete Issues with old (unsupported) versions of LLVM
Projects
None yet
Development

No branches or pull requests

7 participants