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

New tray window #1565

Merged
merged 126 commits into from Jan 18, 2020
Merged

New tray window #1565

merged 126 commits into from Jan 18, 2020

Conversation

DominiqueFuchs
Copy link
Contributor

@DominiqueFuchs DominiqueFuchs commented Oct 27, 2019

This pull request is intended to help with collaborating on specific next steps while developing and testing the new tray window for the nextcloud client ( straight from the hack weekend with @camilasan @misch7 @jancborchardt ). General conception and discussion also in issue #1564 #877

Basic version as of now:
TrayWindow

TO DO (FEATURES):

  • Generate basic UI for the tray window
  • Implement the UI with the new qml tray icon (replacing the one from the widgets) (Camila)
  • Implement activity list model in the UI
  • Connect qml activity list to backend for sync engine
  • Implement a floating menu for header buttons
  • Connect backend with UI regarding account list
  • Include app notifications in the activity list

TO DO (BUGS):

  • Dummy delegate appears (sometimes?) (empty activity at the end of the activitylist)
  • QML add-animations lag/distort when populating many items in short time (extremely visible on first view when a lot of activities are loaded)

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
@DominiqueFuchs DominiqueFuchs added 1. to develop design enhancement enhancement of a already implemented feature/code epic feature: 👥 sharing Sharing dialog and functionalities. feature: ☁️ system tray System tray icon and menu. feature: ⚡ activity and 🔔 notification labels Oct 27, 2019
@DominiqueFuchs
Copy link
Contributor Author

I got the header icon files from Jan and also uploaded them here for reference.

Also:

IMG_1381

Also:

Slightly adjusted font sizes. We'll have to look into the final wording for the second 'username' lane, actually displaying an email adress. The triangle will be positioned vertically centered (based on the header background height) and I'll implement a slight, flat hover effect for the buttons.

Regarding the activity list: Let's prioritize this implementation asap, while staying at a minimum level first (meaning: plain collection of entries with link etc.)

@camilasan camilasan mentioned this pull request Oct 27, 2019
@DominiqueFuchs
Copy link
Contributor Author

headericons.zip

Camila San and others added 2 commits October 27, 2019 20:02
This is a work in progress.

Signed-off-by: Camila San <hello@camila.codes>
Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
@jancborchardt
Copy link
Member

Additional info on which relevant action button we can show for the timeline entries:

  • Server notifications: actions from there
  • Conflict: "Resolve" opening the folder of the file in question
  • Ignore list: "Modify" opening the ignore list settings
  • Any synced file: "Share" which either opens the share menu, more easily directly gives a read-only share link
  • An odt/ods/odp if richdocuments is installed on server, or an md/txt if Text is installed on server: "Edit"

But as discussed, actions is not part of MVP so this is mainly to not forget about it. :)

…iple monitors on windows), integrated new icons in qml

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
@DominiqueFuchs
Copy link
Contributor Author

Integrated the new icons as requested by @jancborchardt (any important change requests regarding sizes / spacings / colors or something at this point?) and (finally) finished the hover effects for the header geometry:

NC_trayMenuHeaderBar

Next on my list is modeling a qml version of the activity list

@DominiqueFuchs
Copy link
Contributor Author

DominiqueFuchs commented Oct 29, 2019

Btw.: Even though this is no prio atm I want to share a thought on the issue what to display as second account label as discussed on the hack weekend. IMHO both a Name („Dominique“) and the Email address can become pretty useless in terms of information content when there are multiple accounts - as the name will most probably the same and the email may be the same.

What about displaying the server address as [subdomain].[domain].[tld]? In most cases this will be not more chars than the max width can contain, otherwise we can [...] it and it still will be enough to see which instance is currently active.

Only downside remaining is the use case where a user is connected to an instance with different accounts. However, for the desktop client I can only imagine test/dev scenarios where this would happen.

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
@DominiqueFuchs
Copy link
Contributor Author

Initial design is ready:

NC_trayMenuListViewBasic

Icons are only temporary obviously. Things that are intended at this point (thus specifically open for discussion/requests):

  • I implemented some animations regarding add/removal/displacement - like or no?
  • Files activity in general: Image (file type), activity (upper text label), file/dir name (lower label), buttons
  • For the file activity, I'd imagine having there two buttons as shortcuts to share link and local file/dir location. I positioned them to be visually in line with the two rightmost buttons on the header menu
  • Listview is scrollable, atm without visible scrollbar - do we need to display one for UX/intuition reasons? When list is fully populated (in terms of: enough items to fill the whole tray window), the last one is visible only partially to indicate it goes further down

Next on my list: Actual connection to cpp backend to show real data

@jancborchardt
Copy link
Member

jancborchardt commented Nov 4, 2019

WOW! Didn’t look at Github for a few days and am blown away. :D

New heading: sick! Really nice, no important changes needed.

Your point about what to show in the second row: 👍 absolutely right.

The design of the activity list: First of I’d like to point out that it all lines up with the header, super nice 😍

  • I implemented some animations regarding add/removal/displacement - like or no?

Very good! The removal animation could be improved a tiny bit since the item vanishes directly (while the collapse is smooth), maybe a quick fadeout or collapse would do the trick?

  • Files activity in general: Image (file type), activity (upper text label), file/dir name (lower label), buttons

Yes, there we also have to check out the lower label. For example if it’s edited by someone else, that’s more important to show than the folder. In that case, it could also show their avatar overlaid on the bottom right of the file preview. (Just realized I totally forgot that during the hackweekend … but yes, kind of like the sync icon would be overlaid on the bottom right.) Makes sense?

  • For the file activity, I'd imagine having there two buttons as shortcuts to share link and local file/dir location. I positioned them to be visually in line with the two rightmost buttons on the header menu

Until we have a 3rd action we could try going for the 2 icons shown directly. But local file location is not so important I’d say. And e.g. for odt or md files, we would also have the "Edit" functionality as per #1564
So as soon as we have that, we should go with 1 action outside (share) and a 3-dot menu, as multiple icons next to each other without labels are not very recognizable.

  • Listview is scrollable, atm without visible scrollbar - do we need to display one for UX/intuition reasons? When list is fully populated (in terms of: enough items to fill the whole tray window), the last one is visible only partially to indicate it goes further down

Good question, I would say yes since people are used to scrollbars at least as an indicator.

Really really awesome work! Check it out @camilasan @misch7 :)

@karlitschek
Copy link
Member

Super awesome work indeed!!!

…uential opacity change and displacement animation

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
…yIcon

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
…endless fetch bug)

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
misch7 and others added 7 commits January 17, 2020 20:16
Signed-off-by: Michael Schuster <michael@schuster.ms>
Signed-off-by: Michael Schuster <michael@schuster.ms>
Signed-off-by: Michael Schuster <michael@schuster.ms>
Signed-off-by: Michael Schuster <michael@schuster.ms>
Signed-off-by: Michael Schuster <michael@schuster.ms>
Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
Signed-off-by: Michael Schuster <michael@schuster.ms>
Copy link
Member

@misch7 misch7 left a comment

Choose a reason for hiding this comment

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

Amazing work @DominiqueFuchs !!! 🎉 👍 😺

Soooo now it's finally time to merge and showcase it! :-)

@misch7 misch7 merged commit 2039872 into master Jan 18, 2020
@misch7 misch7 deleted the qml-tray-menu branch January 18, 2020 12:18
@jancborchardt
Copy link
Member

Historical moment! 🎂 Awesome work @DominiqueFuchs, looking forward to the next hackweek with everyone! :)

@yan12125
Copy link
Contributor

Awesome! I can't wait to try it out, but got a crash:

(gdb) bt
#0  0x00007fffee290f25 in raise () at /usr/lib/libc.so.6
#1  0x00007fffee27a897 in abort () at /usr/lib/libc.so.6
#2  0x00007fffee7f696c in  () at /usr/lib/libQt5Core.so.5
#3  0x00007fffee7f5d52 in  () at /usr/lib/libQt5Core.so.5
#4  0x0000555555605fc4 in QList<OCC::User*>::operator[](int) (i=0, this=0x555555d7c410) at /usr/include/qt/QtCore/qglobal.h:1053
#5  0x0000555555605fc4 in OCC::UserModel::currentActivityModel() (this=0x555555d7c400)
    at /usr/src/debug/nextcloud-client/src/gui/tray/UserModel.cpp:752
#6  0x00005555556e6efa in OCC::Systray::create() (this=0x555555d45bb0) at /usr/src/debug/nextcloud-client/src/gui/systray.cpp:75
#7  0x0000555555641c7e in OCC::Application::Application(int&, char**)
    (this=0x7fffffffdbe0, argc=<optimized out>, argv=<optimized out>) at /usr/include/c++/9.2.0/bits/atomic_base.h:413
#8  0x000055555560cf76 in main(int, char**) (argc=<optimized out>, argv=0x7fffffffddd8)
    at /usr/src/debug/nextcloud-client/src/gui/main.cpp:68

It is a new installation (no accounts configured). Maybe that is relevant?

Tested commit 7136d92 on Arch Linux with Qt 5.14.0.

@misch7
Copy link
Member

misch7 commented Jan 18, 2020

@yan12125 That's super helpful, thank you! 👍 :-)

I'm just trying to prepare everything for the Beta 1 builds, so to know this can only help :)

Edit:
Does this work? ;-)

https://download.nextcloud.com/desktop/prereleases/Linux/Nextcloud-2.7.0.20200118-beta-1-x86_64.AppImage

@DominiqueFuchs
Copy link
Contributor Author

Hi @yan12125 Thanks a lot for this incredibly helpful and fast report :D

As I cant directly commit to master I published a new branch with this fix:

https://github.com/nextcloud/desktop/tree/hotfix-startup-wo-accounts

/ cc @misch7 I think this should be in the first beta, as starting without accounts for testing is pretty common I guess

@misch7
Copy link
Member

misch7 commented Jan 19, 2020

@DominiqueFuchs I've cherry-picked your commit to master 👍 🚀

So this will be in the first beta :)

@yan12125
Copy link
Contributor

Thanks! I can confirm logging in with 2.7.0-beta1 works fine!

Here I have a question: under KDE the location of the tray window looks strange - top right instead of bottom right. Is it expected?

screenshot-ba0f4069

@DominiqueFuchs
Copy link
Contributor Author

@yan12125 Thanks again, that testing is incredibly helpful. 🙇 There is a block of backend logic specifically for the determination of the taskbar and tray position (bottom, left, right, top, any combination) - unfortunately, there seems to be a bug for this specific environment. I'll look into this, stay tuned!

@misch7
Copy link
Member

misch7 commented Jan 20, 2020

Thanks! I can confirm logging in with 2.7.0-beta1 works fine!

@yan12125 Cool, thank you again for reporting and pointing out the position issue :-)

Btw.: From your screenshot I see that you use the version I previously linked above, we've added a few commits to the final Beta 1 build:
https://download.nextcloud.com/desktop/prereleases/Linux/Nextcloud-2.7.0.20200119-beta1-x86_64.AppImage

@DominiqueFuchs fixed the crash you reported 👍

@DominiqueFuchs
Copy link
Contributor Author

OK, that was an ugly one :/ However, fix for this out as PR #1744 👍

// @misch7 @yan12125

@tobiasKaminsky
Copy link
Member

From my experience it is better to have multiple issues instead of commenting on an already merged PR. This way you might lose track of issues.

(just my two cents, as I encourage this on Android repo; not sure how it is handled here)

@DominiqueFuchs
Copy link
Contributor Author

@tobiasKaminsky You‘re right, generally it‘s the same here and helps a lot in maintaining overview and prioritization. Just got used to this thread so much 😄

@yan12125 Feel free top open a new issue in this repo, if you‘ll stumble over a bug. And again: Thanks a lot for testing this beta!

@realies
Copy link

realies commented Jan 28, 2020

Adding dark mode would be great too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. to review design enhancement enhancement of a already implemented feature/code epic feature: 👥 sharing Sharing dialog and functionalities. feature: ☁️ system tray System tray icon and menu. feature: ⚡ activity and 🔔 notification high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants