Multiple messages in the SnackBar

[yogeshvar]

Thanks to @EduRibeiro00 & @X1nbb for documenting the findings and working on this issue. ❤️

Previous Discussion for Handling Multiple Messages: #7577 #7968

From the Snackbars Design Guidelines the frequency of the Snackbar should be only once at a time is recommended. I believe the implementation of https://github.com/sshahine/JFoenix/blob/2dbb8515da236140c41eabe22eb549b458f1e867/jfoenix/src/main/java/com/jfoenix/controls/JFXSnackbar.java#L321 is also based on that. The ideal choice to show Multiple Messages would be moving from Snackbar to Notifications (with custom CSS changes to replicate snack bar design)

Proposed Solution: Moving from Snackbar to Notifcations

Fixes #7340

Notifications notificationBuilder = Notifications.create()
        .text(message)
        .position(Pos.BOTTOM_CENTER)
        .threshold(5, thresholdNotification)
        .hideAfter(TOAST_MESSAGE_DISPLAY_TIME)
        .hideCloseButton();
notificationBuilder.show();

notify.mp4

Few Open Questions while Implementation:

• The Maximum Notification is set as 5, as per the current draft PR. I have displayed a threshold notification that says "Too much happening... Hang on.." Can someone suggest a better UX/message/way handle this, let's discuss.
• For a different workflow, I tried accessing the inner class of Notification. And ended up with this error “Unable to make private void org.controlsfx.control.Notifications$NotificationPopupHandler.hide(javafx.stage.Popup,javafx.geometry.Pos) accessible: module org.controlsfx.controls does not "opens org.controlsfx.control" to module org.jabref”. If someone suggests how to resolve this? I thought of removing the threshold notification and hiding the top of the stack with the help of this while inserting the next incoming notification.

PR Checklist:

• 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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
• Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

[calixtus]

Hi @yogeshvar, thanks for your PR, whick looks very promising.
About your questions:

• The big advantage is, that more than 1 message can be displayed. Maybe there is a way to close the least recent message after the fifth message instead of a new message?
• This seems to be a problem of the java module system. You probably have to edit module-info.java to get access to the NotificationPane? There should be examples inside.

[Siedlerchr]

For the java open module problem you need to modify the build.gradle file and add a line to the addOpens for the concrete class

        addOpens = [
                'javafx.controls/javafx.scene.control' : 'org.jabref',
                'org.controlsfx.controls/org.controlsfx.control.textfield' : 'org.jabref',
                'javafx.controls/com.sun.javafx.scene.control' : 'org.jabref',

[yogeshvar]

Thanks for the quick response, @calixtus @Siedlerchr 🍾 . I have resolved the module issue after modifying the addOpens arg. I was able to successfully access the private method, but I'm not sure about the processing with this solution as it might not be a good practice to get to this level deep. (I wonder if there's a different way.)

try {
      Class<?> clazz = Class.forName("org.controlsfx.control.Notifications");
      // get declared class
      Class<?>[] classes = clazz.getDeclaredClasses();
      for (Class<?> c : classes) {
          if (c.getName().contains("PopupHandler")) {
              // get declared methods
              Method[] methods = c.getDeclaredMethods();
              for (Method m : methods) {
                  if (m.getName().contains("hide")) {
                      m.setAccessible(true);
                  }
              }
          }
      }
  } catch (ClassNotFoundException e) {
      e.printStackTrace();
  }

Also, I have investigated the NotificationPane component from controlsFx which wouldn't be ideal as the notification stays inside the application window. (most likely to be tab alike UI)

I had the idea of adding onAction function to the notification and doing nothing. We could stimulate a mouse click on the notification to close it. Any comments on this?

[calixtus]

We try to avoid reflection if possible and we are accepting tradeoffs, as in the veeeeery long run we wanted to play a bit with GraalVM around.

[yogeshvar]

I'm making this PR ready with the removal of the threshold notification because the notification timeout is 3 seconds which helps the notification to fade out and the new notifications are not restricted by the threshold notification.

Few other findings:

These are the few other works/ideas that I would like to document for future reference if anyone works on this.

• Accessing the hide function from the Notification ControlFx needs reflection. (Try to avoid reflections)
• NotificationPane will be inside the application window, so it wouldn't fit this story.
• With a help of Queue, moving to the Observer pattern can be an option. Although, we are back to square one. (hide function). With more code, we could write an Observer pattern that also holds the Timer (Date object to find the notification duration every second, then remove it.) with every notification in the queue. It might lead to weird fading in and out notifications as we will push all the notifications.
• Returning null with doAction and when the user clicks on the notification it will be closed. (Not a good solution, but it is an alternative for hide) Here are the rough x,y coordinates that I could find for the 5 notifications stacked. (x is approx. always 1024 & y is 745 for the first notification and the rest falls by 65+ approx.)

[calixtus]

Normally it should be sufficient to document your thoughts in this GitHub PR or in the corresponding issue, since the issue number should be recorded in the merge commit message.

[calixtus]

Alternatively you could maybe introduce a short paragraph in the development documentation.

[calixtus]

Is this ready-for-review?

[yogeshvar]

Yes @calixtus

[calixtus]

As this includes a ui change, can you please add a small note in the changelog?
Codeweise looks good to me so far!

[yogeshvar]

@calixtus Thanks, I have updated the changelog, and please let me know whether the note is fine. (I'm conflicted between added/fixed.)

[Siedlerchr]

Thanks for your contribution!