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

Fix pasting on mac and linux #6419

Merged
merged 5 commits into from
Aug 2, 2020
Merged

Fix pasting on mac and linux #6419

merged 5 commits into from
Aug 2, 2020

Conversation

LinusDietz
Copy link
Member

@LinusDietz LinusDietz commented May 4, 2020

PR #6313 caused the double pasting issue on MacOS (see #4699)

After the first paste, the Edit Action focusOwner did somehow trigger on Mac and this resulted in executing paste twice.

Fixes #4699.

This PR caused the double pasting issue on OSX (see #4699)

This reverts commit 5ed32e1.
@LinusDietz LinusDietz added os: macOS status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels May 4, 2020
@LinusDietz LinusDietz added this to the v5.1 milestone May 4, 2020
@LinusDietz LinusDietz requested a review from calixtus May 4, 2020 19:54
@calixtus
Copy link
Member

calixtus commented May 4, 2020

I really don't get where the bug is. Reverting this reopens issue #6293
Especially because, by logic, this should be here. So where is the bug. This should be fixed before 5.1.

@LinusDietz
Copy link
Member Author

LinusDietz commented May 4, 2020

@calixtus Whoops, I overlooked the original issue (#6293) for your PR (#6313).

So, we are in some kind of situation, where we have contradicting behavior in two operating systems. That kinda sucks, since we need to differentiate between those now. :/

Last time, this was the fix: #5210, but maybe this is when we also introduced #6293?!

@calixtus
Copy link
Member

calixtus commented May 4, 2020

I have been thinking this through since the hint of @Siedlerchr. The thing is, the insert command should stand here. Is there another issue with macos, some method fired by macos we did not see yet?

@Siedlerchr
Copy link
Member

If we don't find out the root cause in mac, we could add an ugly os check for Linux/mac

@calixtus
Copy link
Member

calixtus commented May 4, 2020

I already stumbled upon other methods (JabRefFramein::openAction, quit and about) especially for macOS. Did I mention that I don't like apple? 😉

@tobiasdiez
Copy link
Member

I agree with @calixtus, the command there makes sense. @LinusDietz can you debug and report which other code triggers the paste command on MacOS?

@tobiasdiez tobiasdiez added status: changes required Pull requests that are not yet complete and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels May 9, 2020
@Gena928
Copy link
Contributor

Gena928 commented May 10, 2020

OS: macOS Catalina 10.15.4 (19E287)

Hello everyone,
I'm currently trying to reproduce the error of non working menu items, or duplicating objects during "paste" command (from Google Scholar and between two .bib libraries) but I can't.

I use IntelliJ Idea to start JabRef.
Are there any issues with copy/paste?

@koppor
Copy link
Member

koppor commented May 12, 2020

@Gena928 Just to double check: You are on latest master (https://github.com/JabRef/jabref)? @LinusDietz Maybe you can add a screencast using Loom?

@Gena928
Copy link
Contributor

Gena928 commented May 12, 2020

@koppor yes, I'm on latest master.
I downloaded last build and pulled changes from github. Both (last build and source code in IntelliJ) show no error. I.e. record inserts without duplicates.

@Gena928
Copy link
Contributor

Gena928 commented May 12, 2020

Looks like I see an error.
It's very phantom, i.e. I still can not understand what should I do to repeat it. Sometimes I have 2 entries inserted, sometimes only one. I'll continue testing and try to do something about it.

@Gena928
Copy link
Contributor

Gena928 commented May 12, 2020

OK, I need some help )). Looks like paste() command of MainTable.java executed twice:

  1. In EditAction.java, and this was deleted by current pull request. Now it's back again?
    Line 78:
    frame.getCurrentBasePanel().paste();
  2. In MainTable.java method setupKeyBindings;

I tracked first case, and it turns out that EditAction.execute() called by JabRefFrame.java, line 740 (createMenu() method). When I commented that line, JabRef inserts only one item per each Ctrl+V command.

So in general, paste command is called:

  • first time because we have MainTable.setupKeyBindings, and it triggered by Ctrl+V;
  • second time because Ctrl+V command triggers main menu command (Edit=>Paste);

@calixtus
Copy link
Member

calixtus commented May 17, 2020

Sorry for our late reply.
Thank you for you work in tracing this bug.
The deletion of frame.getCurrentBasePanel().paste(); was the unsuccessful attempt to fix the issue in this PR.

Would you like to create a fix for this issue following your investigations and to open a new PR, so we can probably close this one and merge your's with the master branch?

@Gena928
Copy link
Contributor

Gena928 commented May 18, 2020

@calixtus
I hoped to get some Ideas about solving this issue... OK, if there are no any ideas, I will continue to investigate myself.
We have to disable main menu from listening "Ctrl+V" and keep it listening in MainTable.java.

I'll be back with ideas.

@calixtus
Copy link
Member

Does Ctrl + C or Ctrl + X also call an edit action? Maybe there is some design flaw in a hotkey, that is defined twice by setupkeybindings and the the menu command...

@Gena928
Copy link
Contributor

Gena928 commented May 18, 2020

Ctrl+X works in the same way

  1. When I press "Command + X" the program deletes 2 (two) entries from current lib;
  2. Then I move the cursor to another lib and press "Command + V";
  3. JabRef inserts last entry to the new library. I.e. one entry just disappears.

It looks like JabRef cut entries twice...
Checked from IntelliJ and using last stable version from JabRef.org.

@Gena928
Copy link
Contributor

Gena928 commented May 18, 2020

Why do we need SetupKeyBindings method in MainTable.java?
When I delete COPY/PASTE/CUT commands from it, the program works fine.
this lines:

                    case PASTE:
                        paste();
                        event.consume();
                        break;
                    case COPY:
                        copy();
                        event.consume();
                        break;
                    case CUT:
                        cut();
                        event.consume();

So, why do we need those commands in MainTable.java?

@Siedlerchr
Copy link
Member

Siedlerchr commented May 18, 2020

The reason is that you can define custom keybindings (options? manage keybindings) for all commands and so people can use their emacs/vim whatever paste command style.
For mac we use the keybindings and simply replace ctrl with meta in the KeybindingRepository
Maybe that is part of the problem as well?

   public KeyCombination getKeyCombination(KeyBinding bindName) {
        String binding = get(bindName.getConstant());
        if (OS.OS_X) {
            binding = binding.replace("ctrl", "meta");
        }

        return KeyCombination.valueOf(binding);
    }

@Gena928
Copy link
Contributor

Gena928 commented May 18, 2020

@Siedlerchr
if I remove this line of code. binding = binding.replace("ctrl", "meta"); I just have to use "Command" button on Mac, instead of "Control". A bit more natural for IOS.
The problem persists.

What about main menu?
I mean, if you wanna change key combinations, and we remove that lines from MainTable.java. Does main menu works with new key combinations for COPY/PASE/CUT?

@Siedlerchr
Copy link
Member

I will test this on Windows : Options -> Customize Keybindings:
grafik
I changed copy/paste to the following (Alt Gr+O and Alt GR+P)
I can copy and paste using those shortcuts.
CTRL+C and CTRL+V then don't work.

I also tested uncommenting the setupKeyBindings in MainTable.
No difference, no duplicates.

So, now we would need someone on Linux who can test this.

@Gena928
Copy link
Contributor

Gena928 commented May 18, 2020

Linux user wanted! :-)

@Siedlerchr
Copy link
Member

@LyzardKing Could you assist in testing this on linux? AssiGg some other shortcuts to copy/paste
test if it works
And then do a test with commenting out the method setupKeyBindings in MainTable

@LyzardKing
Copy link
Collaborator

Sure, I can test both stable and the version from builds.jabref.org
It's not clear to me what I should be doing..
I've never had duplicates in pasting... if that's the issue here.

@calixtus
Copy link
Member

calixtus commented Jun 5, 2020

Issue #6293 was about merging in pasting in linux and windows. Lets follow the mac related stuff here.

@Siedlerchr
Copy link
Member

Will look into this as I have now a mac at hand

@Siedlerchr
Copy link
Member

Nope, didnt fix it. But I think I found the real reason now

@LyzardKing
Copy link
Collaborator

Doesn't work on linux.
I hadn't noticed this issue before.

@LyzardKing
Copy link
Collaborator

In the .deb it now doesn't work even if I press Ctrl+V

@Siedlerchr
Copy link
Member

I think I have found the root cause now. Pasting to a new library should work now as well. Please try again when the new build is ready.

@Siedlerchr
Copy link
Member

new build is ready , please try pasting an entry from a library to a new empty library
https://builds.jabref.org/pull/6419/merge/

@LyzardKing
Copy link
Collaborator

It works, but I have top copy Ctrl-V multiple times to get it to work if the library is empty.
The copy/paste buttons are not active on an empty library
On a not empty library it works normally.

@Siedlerchr Siedlerchr changed the title Revert "Fixed missing paste command (#6313)" Fix pasting on mac and linux Aug 1, 2020
@Siedlerchr Siedlerchr modified the milestones: v5.2, v5.1 Aug 1, 2020
@Siedlerchr Siedlerchr added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: changes required Pull requests that are not yet complete labels Aug 1, 2020
@Siedlerchr
Copy link
Member

Siedlerchr commented Aug 1, 2020

Works fine on Windows as well. The menu buttons are disabled on Windows as well. I think they are only activated if you have at least one entry selected. But this is a minor issue.
Therefore I would propose to merge this.

@koppor
Copy link
Member

koppor commented Aug 1, 2020

Regarding the disabled/enabled menu items, we track the issues at koppor#419.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

I think, it's good to go.

We need to re-open #6293 as this is not fixed.

@@ -73,7 +73,6 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We fixed an issue where the INSPIRE fetcher was no longer working [#6229](https://github.com/JabRef/jabref/issues/6229)
- We fixed an issue where custom exports with an uppercase file extension could not be selected for "Copy...-> Export to Clipboard" [#6285](https://github.com/JabRef/jabref/issues/6285)
- We fixed the display of icon both in the main table and linked file editor. [#6169](https://github.com/JabRef/jabref/issues/6169)
- We fixed the paste entry command in the menu and toolbar, that did not do anything. [#6293](https://github.com/JabRef/jabref/issues/6293)
Copy link
Member

Choose a reason for hiding this comment

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

We should also reopen the issue as the edit->paste menu entry doesn't do anything.

@Siedlerchr
Copy link
Member

The problem turns out to be a bit complex. We need to find a way to differ if an action was triggered by key or not or to move the keybinding from the maintable to the edit action.
Therefore I would prefer to have this PR in as it enables copy pasting vai keyboard commands and we can reopen the other issue regarding the menu commands

@Siedlerchr Siedlerchr merged commit 78aed32 into master Aug 2, 2020
@Siedlerchr Siedlerchr deleted the fix-4699-again branch August 2, 2020 11:40
Siedlerchr added a commit that referenced this pull request Aug 9, 2020
* upstream/master: (47 commits)
  Fix copy pasting and delete via menu or key (#6740)
  Add instructions how to work with fetchers  (#6731)
  Autoinstall extension in chrome (#6442)
  Delete link after download (#6723)
  New translations JabRef_en.properties (Portuguese, Brazilian) (#6728)
  Bump pascalgn/automerge-action from v0.8.5 to v0.9.0 (#6736)
  Bump byte-buddy-parent from 1.10.13 to 1.10.14 (#6733)
  Bump mockito-core from 3.4.4 to 3.4.6 (#6734)
  Bump unirest-java from 3.8.06 to 3.9.00 (#6735)
  Bump org.beryx.jlink from 2.21.1 to 2.21.2 (#6732)
  Add testing interface, including a set of capabilities to tests for (#6687)
  Fix pasting on mac and linux (#6419)
  Add validation of "AUTHORS" file (#6722)
  Squashed 'src/main/resources/csl-styles/' changes from cacc4ee..827b986
  New Crowdin updates (#6721)
  Add missing AUTHORs
  Fix for issue 6639 (#6719)
  Fix more links
  Fix link
  New Crowdin updates (#6718)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os: macOS status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mac OS X: Pasting bibtex entry creates duplicate entries
7 participants