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 ID Fetcher in Entrytypedialog #1925

Merged
merged 16 commits into from
Sep 12, 2016
Merged

Conversation

zesaro
Copy link
Contributor

@zesaro zesaro commented Sep 5, 2016

Add an extra field in the EntryTypeDialog to fetch the ID Based fetcher.
The field is in on the bottom of the dialog and is named ID based generator.

Normal
dialog30-f

Searching
dialog31-f

No internet connection
dialog34-f

Error message
fetcher35-f

@zesaro zesaro changed the title Entrytypedialog Add ID Fetcher in Entrytypedialog Sep 5, 2016
@Siedlerchr
Copy link
Member

To make it a bit clearer for the user, I would rename it "From ID based fetcher".
So the semantic workflow could be "create new entry from isbn 12346567"

@oscargus
Copy link
Contributor

oscargus commented Sep 6, 2016

Or just "From ID".

I think the idea is really good, but I'm thinking that there may be a better location, like a separate menu item. However, I assume that this is the dialog shown when pressing "new entry" in the toolbar (which I rarely use). No really good idea, but later it may be worthwhile trying to e.g. have a side panel with that functionality (or rather separating the fetcher types in search and ID, I realize).

@@ -1640,3 +1640,8 @@ last_four_nonpunctuation_characters_should_be_numerals=
Remember_password?=Kom_ihåg_lösenord?

shared=delad
Remember_password?=
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate. :-)

@tobiasdiez
Copy link
Member

I'm also in favor of a new menu-item + dialog. This also provides you more room to include a bit more details. My proposal:
Image

Has the advantage that the user need not select the kind of ID he has (most of them are quite distinct and JabRef should be able to tell them apart) and he gets a preview of the entry (which tells him that we put the correct ID in)

@koppor
Copy link
Member

koppor commented Sep 6, 2016

@tobiasdiez I like the idea, but @zellerdev should do it in addition. I like the current dialog by @zellerdev. I vote for doing both!

I like the current "ID-based entry generator" (@zellerdev please rename acoordingly). Rename "Search" to "Generate". Is it possible to place "Cancel" to the right?

@zesaro zesaro changed the title Add ID Fetcher in Entrytypedialog [WIP] Add ID Fetcher in Entrytypedialog Sep 6, 2016
@Override
protected void done() {
if (isCancelled()) {
JOptionPane.showMessageDialog(null, "Fetching was canceled", "Canceled fetching", JOptionPane.INFORMATION_MESSAGE);
Copy link
Member

Choose a reason for hiding this comment

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

Why is the mesasge not localized?

@koppor
Copy link
Member

koppor commented Sep 7, 2016

Please add updated screenshot, also the German screenshot to be sure that everything is translated. Ater that, it should be good to go.

import org.jdesktop.swingx.VerticalLayout;

import static net.sf.jabref.gui.importer.fetcher.EntryFetchers.getIdFetchers;
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if you don't import this as static.

dispose();
} catch (FetcherException e) {
LOGGER.error("Error fetching from " + fetcher.getName(), e);
JOptionPane.showMessageDialog(null, Localization.lang("Error_while_fetching_from_%0", fetcher.getName()), Localization.lang("Error_while_fetching"), JOptionPane.ERROR_MESSAGE);
Copy link
Member

@tobiasdiez tobiasdiez Sep 7, 2016

Choose a reason for hiding this comment

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

without underscore and also display error message.

@tobiasdiez
Copy link
Member

Just some small remarks, then this could be merged.

@@ -2177,7 +2177,7 @@ Copied_version_to_clipboard=Copied_version_to_clipboard

BibTeX_key=BibTeX_key
Message=Message

ID-based_entry_generator=ID-based_entry_generator
Copy link
Member

Choose a reason for hiding this comment

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

The order of entries in JabRef_en.properties is important. Please move this together with No_entry_with_id_'%0'_for_fetche.... Maybe at the end of the file (as it is common for new entries)

@koppor
Copy link
Member

koppor commented Sep 12, 2016

Can you rewrite the code to depend on #1885 only. As #1923 depends on #1929, it will take more than this week to get it merged. I don't want to wait that long. - Update I think, you code does not depend on these two fetchers, but only your initial comment at this PR. Maybe just comment at these PRs that they should update EntryFetchers.getIdFetchers(). Reason: This PR seems (besides minor comments) ready to be merged and we should not postpone it to avoid unnecessary conflict resolutions.

@koppor koppor changed the title [WIP] Add ID Fetcher in Entrytypedialog Add ID Fetcher in Entrytypedialog Sep 12, 2016
@koppor koppor merged commit 793ec23 into JabRef:master Sep 12, 2016
@zesaro zesaro deleted the entrytypedialog branch September 12, 2016 20:16
@zesaro zesaro restored the entrytypedialog branch September 15, 2016 15:11
zesaro added a commit to zesaro/jabref that referenced this pull request Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants