search

chewing / chewing-editor

Cross platform chewing user phrase editor
https://chewing.im/
GNU General Public License v2.0
31 stars 52 forks source link

Fix to show correct number of imported phrases #126

Closed cyng93 closed 8 years ago

cyng93 commented 8 years ago

fix, or probably workaround for issue #49. not sure if this happen only on Ubuntu or other distro are also having the same issue, but not using native file dialog seems to fix the problem in Ubuntu.

References: https://bugreports.qt.io/browse/QTBUG-38985, http://forum.qt.io/topic/41356/qfiledialog-signal-fileselected-is-triggered-twice-for-one-user-choice-changed-in-recent-qt/2

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 94.382% when pulling 0cc8e545f16536b8a1c5b6067413d6a6e45ec33b on cyng93:master into f7f3123b4c3497649792cbc78318d504f540aa90 on chewing:master.

Chocobo1 commented 8 years ago
  1. DO provide screenshots for UI changes, before & after.
  2. It's 2016 already, who uses Qt built-in styles? this workaround is not preferable.
  3. This issue is not in windows. I confirm in linux.
  4. Notification::notifyImportCompleted is signaled twice for unknown reason, maybe just add a blocking condition when result == 1 && imported == 0 is enough.
coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 94.382% when pulling 965f3408c0dbbf874121d397bf5ddc97455db214 on cyng93:master into f7f3123b4c3497649792cbc78318d504f540aa90 on chewing:master.

cyng93 commented 8 years ago

DO provide screenshots for UI changes, before & after.

I remove all entries before import "chewing.json", and here's the screenshot after I imported.

  • Before: selection_044
  • After: selection_045

Not sure if this kind of screenshot are the one I should attached, let me know if it's not and I'll try to fix it.

It's 2016 already, who uses Qt built-in styles? this workaround is not preferable.

In the new commit, i use a counter to avoid double trigger, please check.

This issue is not in windows. I confirm in linux.

By linux, does it mean all linux distro are having this isuse, or particular on Ubuntu?

Notification::notifyImportCompleted is signaled twice for unknown reason, maybe just add a blocking condition when result == 1 && imported == 0 is enough.

Thanks for the advice! It's really useful.

Chocobo1 commented 8 years ago

In the new commit, i use a counter to avoid double trigger, please check.

still... a counter is too much IMO.

By linux, does it mean all linux distro are having this isuse, or particular on Ubuntu?

I only tested on my setup which is archLinux + cinnamon DE + Qt 5.5.1.

Thanks for the advice! It's really useful.

Really, it's more than just an advice, I hope you implement it (if it really works).

BTW, you use markdown quote mark in an exact inverse way...

Chocobo1 commented 8 years ago

please do a git rebase.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 94.382% when pulling c77a25887b40c8059411b6e87c94b99d97591982 on cyng93:master into f7f3123b4c3497649792cbc78318d504f540aa90 on chewing:master.

david50407 commented 8 years ago

DO NOT commit .swp files.

IMO, you can delete https://github.com/chewing/chewing-editor/pull/126/commits/56cd2e9ce41441c8cf52f402d204d66dc8fcc979 directly to avoid reverting it. @Chocobo1 did I say right?

Chocobo1 commented 8 years ago

@cyng93 at the time I typing this, I see this PR is messed up. you'll have to know how to use git effectively, especially these: branch, rebase, revert. please take your time and fix it properly.

jserv commented 8 years ago

Please use git rebase -i to rework your changes.

jserv commented 8 years ago

@cyng93 : Can you perform git rebase?

cyng93 commented 8 years ago

Sorry guys for the messy PR as this was my first time handling with all these collaborating work. I am still confuse with some stuff and looking forward for some sort of explanation. First of all, is this PR still mess out in other stuff other than the swp files is included? If the problem is with the swp files, couldn't I just removed the files and push another commit?

DO NOT commit .swp files. IMO, you can delete 56cd2e9 directly to avoid reverting it. Chocobo1 did I say right?

@david50407 I could understand why deleting https://github.com/chewing/chewing-editor/pull/126/commits/56cd2e9ce41441c8cf52f402d204d66dc8fcc979 is an option, as those *swp files was added in https://github.com/chewing/chewing-editor/pull/126/commits/e50861f4450f086d80ba4d5f02156911ca0b4c54.

Please use git rebase -i to rework your changes.

@jserv I had done a git rebase before, so what is the purpose for doing it again? Am I doing it in the wrong way or you're expecting me to solve the *swp file problem using a git rebase?

Sorry again if stupid question was asked, thought asking you guys might be a better options instead of digging around in stackoverflow without understanding what the question I am facing.

jserv commented 8 years ago

@cyng93 : You have to rebase upon the latest GIT commit (so-called "HEAD") of master branch. Reference: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

Chocobo1 commented 8 years ago

@cyng93 Some short instructions, you were doing changes on your local master branch, this increases the difficulty of rebasing operations (for git beginners):

  1. create a local branch like master_tmp to track upstream master branch (i.e. this repo master branch).
  2. revert your local master branch to just contain the changes/commits you intended.
  3. while in master branch, run git rebase master_tmp
  4. fix conflicts if there are any.
  5. while in master branch, git push -f

hope this is clear.

david50407 commented 8 years ago

@david50407 I could understand why deleting 56cd2e9 is an option, as those *swp files was added in e50861f.

@cyng93 these are different things, in https://github.com/chewing/chewing-editor/pull/126/commits/56cd2e9ce41441c8cf52f402d204d66dc8fcc979 commited 

fileDialog_->setOption(QFileDialog::DontUseNativeDialog, true);

for qt dialog, but immediately removed in https://github.com/chewing/chewing-editor/pull/126/commits/e50861f4450f086d80ba4d5f02156911ca0b4c54

I mean, https://github.com/chewing/chewing-editor/pull/126/commits/56cd2e9ce41441c8cf52f402d204d66dc8fcc979 can be removed to avoid reverting changes in https://github.com/chewing/chewing-editor/pull/126/commits/e50861f4450f086d80ba4d5f02156911ca0b4c54

david50407 commented 8 years ago

First of all, is this PR still mess out in other stuff other than the swp files is included? If the problem is with the swp files, couldn't I just removed the files and push another commit?

*swp files usually used for vim (or other editor) temporary files, not related to this project. If you add these in a commit and remove them in another commit, they still in the history.

Before we merge into master, we have to make sure this PR or changes are clean, or the history will be a mess.

jserv commented 8 years ago

@cyng93 : Alternately, you can switch to a new branch and re-do the changes

jserv commented 8 years ago

@cyng93 : I reworked your patch into file fix.zip. Here are the instructions:

$ cd /tmp
$ unzip fix.zip
$ git clone git@github.com:cyng93/chewing-editor.git
$ cd chewing-editor
$ git remote add upstream https://github.com/chewing/chewing-editor.git
$ git fetch upstream
$ git reset --hard 3fe8357c70e1632c1418e83f93fed7d6d4f3b799
$ git rebase upstream/master
$ git am -s /tmp/0001-fix-for-showing-correct-import-count-when-import-suc.patch

When the above is ready, use git commit --amend to improve the git commit messages and git push --force to override the submitted changes.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 94.382% when pulling c0d1557c9dba88023d332d5668806bb1c061c978 on cyng93:master into 5514dfa652e4739d2af1b3a0aaf13b589c3a943d on chewing:master.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 94.382% when pulling e0ac6ba2cec282ed8f9f05e977e7b24035f0632c on cyng93:master into 5514dfa652e4739d2af1b3a0aaf13b589c3a943d on chewing:master.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 94.382% when pulling 7dbc313bbc6e5a4d7fc50b5d4e87d449e3b4df8f on cyng93:master into 5514dfa652e4739d2af1b3a0aaf13b589c3a943d on chewing:master.

jserv commented 8 years ago

@cyng93 If this patch can fix #49 , you can append "Close #49" in the GIT commit messages.

jserv commented 8 years ago

@cyng93 You didn't rebase to latest upstream HEAD. You have to do again.

$ git fetch upstream
$ git rebase upstream/master
cyng93 commented 8 years ago

@jserv , noted ! I'll fixed them. Sorry for making all these notification, perhaps I should try all these things in a new branch first.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 94.382% when pulling fb40690eb4e856f9a88ea7d9434c50ac557da49b on cyng93:master into 5514dfa652e4739d2af1b3a0aaf13b589c3a943d on chewing:master.

jserv commented 8 years ago

@cyng93 : The latest patch looks much better. However, can you improve the English expressions in GIT commit messages? Please read this article How to Write a Git Commit Message again.

jserv commented 8 years ago

@cyng93 : Limit the subject line to 50 characters! Subject line is the first one in GIT commit messages.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 94.382% when pulling 6e5c8567d45c8cc7fecf0378ebbdf266751a2b8c on cyng93:master into c987c8c9d93bbd186c62630312f46bed51bb7e2c on chewing:master.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 94.382% when pulling 8fc29741581d76b356a61cd402bd0174afe68f43 on cyng93:master into c987c8c9d93bbd186c62630312f46bed51bb7e2c on chewing:master.

jserv commented 8 years ago

@cyng93 : Your GIT commit messages are less elastic. Please read How to Write a Git Commit Message. The subject should contain no issue number. Instead, put "Close #???" in the third line.

In addition, you should not mention "Linux" since chewing-editor is a portable program. Your changes are not platform-dependent.

cyng93 commented 8 years ago

@jserv noted, but how could I modify the commit message? Should I use the trick git reset --hard 3fe8357c70e1632c1418e83f93fed7d6d4f3b799 you've mentioned previously again ? I mentioned "Linux" because AFAIK, this bug only occur on Linux.

jserv commented 8 years ago

@cyng93 Use git commit --amend to rework the commit messages. You should analyze the issue and distinguish in order to verify that it is common for POSIX compatible platforms or just Linux specific.

cyng93 commented 8 years ago

@jserv, I spend some time trying to reproduce the bug on other platform, trying to figure out what really cause this bug. The answer is still remain unclear, but I thought someone might be able to come out with somethings so I decided to post the result out first.

For every platform below, I import chewing.json.zip directly after the program was built and check if the bug appeared. Below are the result:

Freebsd

Linux

As far as I could tell, I don't think that this bug is related to Linux as not every desktop environment of Debian (7-9) above has the bug appeared on them.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 94.382% when pulling 4d84fce41bcc150e69724f3d63c62773a9f32730 on cyng93:master into 3e8e39131c9bfb8b33e785d068bc446776bba888 on chewing:master.

jserv commented 8 years ago

@cyng93 : Thanks for clarifying this issue. Appending to GIT commit messages, you can make a summary for known combination of software packages where this patch can resolve.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 94.382% when pulling cc5cd9643583a763592ec5e6993782573c491bae on cyng93:master into 3e8e39131c9bfb8b33e785d068bc446776bba888 on chewing:master.

jserv commented 8 years ago

@cyng93 : I merged your contribution, then we can rework/refactor later.