Feature : delete confirmation window

chewing / chewing-editor

Cross platform chewing user phrase editor

https://chewing.im/

GNU General Public License v2.0

31 stars 52 forks source link

Feature : delete confirmation window #173

Closed YWJamesLin closed 8 years ago

YWJamesLin commented 8 years ago

I think this is important, then I make a simple dialog. Please check my code.

Known Issue of this PR:

Add icon in this window

david50407 commented 8 years ago

I think you should squash these two commits into one.

jserv commented 8 years ago

Never say "The same as title".

Chocobo1 commented 8 years ago

Don't know why you closed this PR. If you're willing to improve/refactor it: I suggest you use QMessageBox instead of creating a new kind of message box.

YWJamesLin commented 8 years ago

Since I don't know how to edit the pull request. I know how to squash commit, but I don't know how to edit PR to use another commit... I will improve that and open another PR...

Chocobo1 commented 8 years ago

For a start, you just write some code and do some changes and then git commit as usual, then when you're satisfy with it, start git rebase -i to squash the commits. Then you git push -f to push your changes to github and leave a message for us to start another round of review.

Chocobo1 commented 8 years ago

I know how to squash commit, but I don't know how to edit PR to use another commit...

When you add new commit to your feature-deleteConfirmWindow branch and then git push, it will also update this PR. In short, branch feature-deleteConfirmWindow is linked with this PR (ONLY when it's open!).

YWJamesLin commented 8 years ago

@Chocobo1 Thank you! I've fixed these problem! I'll try to improve this feature with QMessageBox.

@jserv @david50407 I've edited the content of this PR, and squashed the two commits with indentation fixing. By the way, should I provide a snapshot of this feature?

jserv commented 8 years ago

@YWJamesLin : Can you improve the GIT commit messages to mention what the new feature is?

jserv commented 8 years ago

Also, it is required to rebase. Please do check the documentation about git rebase.

YWJamesLin commented 8 years ago

To all: Now I have refactored the code. @jserv Since I refactor the code, I don't modify the remove() method. I think I just totally do one thing (just add comfirm window) and I may need only one commit. If you think this is not a good choice, please give some advice. @Chocobo1 I have refactor this by using QMessageBox instead of QDialog, please check again.

coveralls commented 8 years ago

Coverage remained the same at 94.382% when pulling 4a9daba5fab21a29f6185639840e712be3a342da on YWJamesLin:feature-deleteConfirmWindow into 5514dfa652e4739d2af1b3a0aaf13b589c3a943d on chewing:master.

Chocobo1 commented 8 years ago

OK for me (and give other maintainers some time to review it).

jserv commented 8 years ago

@YWJamesLin : You can append the issue number associated with this change, and GiHub will automatically close that issue after your contribution is merged. Use git commit --amend to specify.

coveralls commented 8 years ago

Coverage remained the same at 94.382% when pulling 8480f7b5ee4a3515a34832bcf5021708873ae027 on YWJamesLin:feature-deleteConfirmWindow into 5514dfa652e4739d2af1b3a0aaf13b589c3a943d on chewing:master.

jserv commented 8 years ago

The subject should exclude issue number. So, you can rewrite GIT commit messages as following:

add confirmation window while deleting user-phrase

Close #155

coveralls commented 8 years ago

Coverage remained the same at 94.382% when pulling e93a51f00e845d47565e6941d9342661e2871916 on YWJamesLin:feature-deleteConfirmWindow into 5514dfa652e4739d2af1b3a0aaf13b589c3a943d on chewing:master.