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

Added icon picker in group dialog #7776

Merged
merged 16 commits into from
Aug 17, 2021
Merged

Conversation

BasilDimopoulos
Copy link
Contributor

@BasilDimopoulos BasilDimopoulos commented May 30, 2021

Heya guys, I understand that this issue already has a pull request associated, however, we were just hoping to get some feedback on this for our university project. We have developed a UI selector that fills out the icon text field. Fixes #6142 .

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

image

@calixtus
Copy link
Member

calixtus commented Jun 3, 2021

Did you notice, @EricLee543 opened a similar PR #7758 some days ago?
One of the two PRs is probably lost work now...

@calixtus
Copy link
Member

calixtus commented Jun 3, 2021

On first sight: There are several issues mentioned by the reviewdog and seeing your screenshot, something seems of with theming/style of the icon select dialog.

@koppor
Copy link
Member

koppor commented Jun 3, 2021

@BasilDimopoulos Welcome to JabRef development.

The first thing to learn about open-source-projects is that developers have limited time resources. They rely on automatation.

Did you see our automated tools?

grafik

grafik

Based on our long experiences, I see the following things: Your supervisor seem not to have contacted us in advanced (see https://devdocs.jabref.org/teaching for details). Thus, I have to ask you whether you intend to work on this PR for the next weeks?

@tobiasdiez
Copy link
Member

For better performance, one could maybe use https://github.com/FXMisc/Flowless which should be able to handle a large amount of icons (because only a bunch of them are actually created).

Might also be helpful for the maintable actually, which would address @k3KAW8Pnf7mkmdSMPHz27's question!

@Siedlerchr
Copy link
Member

Another suggestion would be to add an interface like this:
Search and display the icons by category (not sure if it's possible with kordamp)

https://icon-sets.iconify.design/mdi/

@calixtus calixtus marked this pull request as draft August 17, 2021 14:59
@calixtus
Copy link
Member

calixtus commented Aug 17, 2021

grafik

grafik

@calixtus calixtus marked this pull request as ready for review August 17, 2021 17:07
@calixtus calixtus requested a review from Siedlerchr August 17, 2021 17:07
@calixtus calixtus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 17, 2021
@calixtus calixtus requested a review from tobiasdiez August 17, 2021 17:12
ObservableList<Ikon> ikonList = FXCollections.observableArrayList();
FilteredList<Ikon> filteredList = new FilteredList<>(ikonList);

ikonList.addAll(allOf(MaterialDesignA.class));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ikonList.addAll(allOf(MaterialDesignA.class));
Set<IkonProvider<Ikon>> result = ServiceLoader.load(IkonProvider.class.getModule().getLayer(), IkonProvider.class).stream()
.filter(provider -> provider.get().getClass() != JabrefIconProvider.class)
.map(Provider::get).collect(Collectors.toSet());
for (IkonProvider provider: result) {
ikonList.addAll(allOf(provider.getIkon()));
}

Copy link
Member

Choose a reason for hiding this comment

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

Something seems to be odd. Works with eclipse, but not with gradlew / javac

                                                      .map(ServiceLoader.Provider::get).collect(Collectors.toSet());
                                                                                               ^
    (argument mismatch; Collector<IkonProvider<Ikon>,CAP#2,Set<IkonProvider<Ikon>>> cannot be converted to Collector<? super IkonProvider,CAP#2,Set<IkonProvider<Ikon>>>)
  where R,A,T#1,T#2 are type-variables:
    R extends Object declared in method <R,A>collect(Collector<? super T#1,A,R>)
    A extends Object declared in method <R,A>collect(Collector<? super T#1,A,R>)
    T#1 extends Object declared in interface Stream
    T#2 extends Object declared in method <T#2>toSet()
  where CAP#1,CAP#2 are fresh type-variables:
    CAP#1 extends Object from capture of ?
    CAP#2 extends Object from capture of ?```

@Siedlerchr Siedlerchr merged commit a80435f into JabRef:main Aug 17, 2021
@calixtus calixtus removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 18, 2021
@calixtus calixtus changed the title Adding icon picker for group dialog issue#6142 Adding icon picker in group dialog Aug 18, 2021
@calixtus calixtus changed the title Adding icon picker in group dialog Added icon picker in group dialog Aug 18, 2021
Siedlerchr added a commit that referenced this pull request Aug 20, 2021
* upstream/main: (110 commits)
  Extract PushTo names into model (#8005)
  Refactor processCitation in GrobidService to match processPdf (#8003)
  Improved progress indication for fulltext-index operations (#7981)
  Reordered Pdf-Importer priorities (#8001)
  Implement more pdf importers (#7947)
  Adding icon picker for group dialog issue#6142 (#7776)
  Fix possible NPE in exporter with empty charset (#7979)
  Fix icon color (#7994)
  Bump slf4j-api from 2.0.0-alpha2 to 2.0.0-alpha4 (#7991)
  Bump classgraph from 4.8.112 to 4.8.114 (#7990)
  Bump mariadb-java-client from 2.7.3 to 2.7.4 (#7992)
  Bump jsoup from 1.14.1 to 1.14.2 (#7993)
  New yaml issue template (#7983)
  [Bot] Update CSL styles (#7985)
  Reordered items in main table right-click menu (#7952)
  Fulltext Index: Only index local pdf files (#7980)
  Bump WyriHaximus/github-action-wait-for-status from 1.3 to 1.4 (#7973)
  Bump byte-buddy-parent from 1.11.9 to 1.11.12 (#7974)
  Bump classgraph from 4.8.110 to 4.8.112 (#7975)
  Bump checkstyle from 8.45 to 8.45.1 (#7978)
  ...

# Conflicts:
#	src/main/java/module-info.java
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.

Add icon picker in group edit dialog
6 participants