nuttyartist / notes

Fast and beautiful note-taking app written in C++. Write down your thoughts.
https://www.get-notes.com
Mozilla Public License 2.0
3.68k stars 319 forks source link

Create kanban view based on editor's text, solves #563 #574

Closed nuttyartist closed 1 year ago

nuttyartist commented 1 year ago

This PR creates a kanban view based on the detected tasks currently in the m_textEdit (text editor).

This is what this PR looks like currently:

test

Remaining tasks for this PR:

Code improvements, known issues and future features are tracked here: https://github.com/nuttyartist/notes/issues/593

zjeffer commented 1 year ago

First of, excuse my bad practice of not creating different commits during development to track changes.

Where I work, we create 1 commit per PR (or multiple commits if a PR contains multiple separate features), so I don't mind this at all. I don't know if there's a universal "best way" to do this.

zjeffer commented 1 year ago

Running the latest commit's AppImage (qt6) throws the following errors on startup:

qrc:/qt/qml/kanbanMain.qml:3:1: module "QtQuick.Controls.Material" is not installed 
     import QtQuick.Controls.Material 
     ^
qrc:/qt/qml/kanbanMain.qml:2:1: module "QtQuick.Controls" is not installed 
     import QtQuick.Controls 
     ^
qrc:/qt/qml/kanbanMain.qml: module "QtQml.WorkerScript" is not installed
qrc:/qt/qml/kanbanMain.qml:3:1: module "QtQuick.Controls.Material" is not installed 
     import QtQuick.Controls.Material 
     ^
qrc:/qt/qml/kanbanMain.qml:2:1: module "QtQuick.Controls" is not installed 
     import QtQuick.Controls 
     ^
qrc:/qt/qml/kanbanMain.qml: module "QtQml.WorkerScript" is not installed
qrc:/qt/qml/kanbanMain.qml:3:1: module "QtQuick.Controls.Material" is not installed 
     import QtQuick.Controls.Material 
     ^
qrc:/qt/qml/kanbanMain.qml:2:1: module "QtQuick.Controls" is not installed 
     import QtQuick.Controls 
     ^
qrc:/qt/qml/kanbanMain.qml: module "QtQml.WorkerScript" is not installed

and the entire editor window is white when clicking the Switch button.

guihkx commented 1 year ago

Yeah, I'm still trying to figure out why linuxdeploy isn't including those automatically...

-------- Original Message -------- On Apr 20, 2023, 15:51, Tuur Vanhoutte wrote:

Running the latest commit's AppImage (qt6) throws the following errors on startup:

qrc:/qt/qml/kanbanMain.qml:3:1: module "QtQuick.Controls.Material" is not installed import QtQuick.Controls.Material ^ qrc:/qt/qml/kanbanMain.qml:2:1: module "QtQuick.Controls" is not installed import QtQuick.Controls ^ qrc:/qt/qml/kanbanMain.qml: module "QtQml.WorkerScript" is not installed qrc:/qt/qml/kanbanMain.qml:3:1: module "QtQuick.Controls.Material" is not installed import QtQuick.Controls.Material ^ qrc:/qt/qml/kanbanMain.qml:2:1: module "QtQuick.Controls" is not installed import QtQuick.Controls ^ qrc:/qt/qml/kanbanMain.qml: module "QtQml.WorkerScript" is not installed qrc:/qt/qml/kanbanMain.qml:3:1: module "QtQuick.Controls.Material" is not installed import QtQuick.Controls.Material ^ qrc:/qt/qml/kanbanMain.qml:2:1: module "QtQuick.Controls" is not installed import QtQuick.Controls ^ qrc:/qt/qml/kanbanMain.qml: module "QtQml.WorkerScript" is not installed

and the entire editor window is white when clicking the Switch button.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because your review was requested.Message ID: @.***>

nuttyartist commented 1 year ago

@guihkx @zjeffer So you both can't test the current branch?

zjeffer commented 1 year ago

@guihkx @zjeffer So you both can't test the current branch?

I think I can by compiling it myself but I didn't have time yesterday.

I might try it out tomorrow.

guihkx commented 1 year ago

Okay, I believe I have fixed the AppImage issue on my last push, so please retest when possible.

@nuttyartist Unrelated to the AppImage itself, I'm getting this message on console whenever I enable the Kanban view:

<Unknown File>: columnStartLine is undefined. Adding an object with a undefined member does not create a role for it.
guihkx commented 1 year ago

Great, now the openSUSE job is failing randomly. :P

It's probably temporary and most definitely unrelated to the change I just pushed, so please ignore that for now.

nuttyartist commented 1 year ago

@guihkx @zjeffer So you both can't test the current branch?

Just to clarify, I mean to ask if you are getting any errors and therefore unable to test (it was not some passive-aggressive question on why you're not testing haha).

nuttyartist commented 1 year ago

Okay, I believe I have fixed the AppImage issue on my last push, so please retest when possible.

@nuttyartist Unrelated to the AppImage itself, I'm getting this message on console whenever I enable the Kanban view:

<Unknown File>: columnStartLine is undefined. Adding an object with a undefined member does not create a role for it.

Weird, let's see if it still persists after I push some stuff.

guihkx commented 1 year ago

Just to clarify, I mean to ask if you are getting any errors and therefore unable to test (it was not some passive-aggressive question on why you're not testing haha).

Too late, I'm already offended. :(

Seriously though, just a reminder to myself that I (or we) should also check if the Windows installer (and the portable version too) are also including the necessary qml files.

I'll leave that one for later though, it's quite late already. :)

guihkx commented 1 year ago

Pushed some fixes for the Windows build. Did a quick test and it everything seems to work now!

My macOS VM is borked again, so I won't be able to test the dmg until I fix it again... But it'd be great to get a confirmation that it works (especially the Qt 6 one, of course).

One small visibility issue I noticed while testing the AppImage: With the dark theme enabled, after you mark a specific task as completed, its text becomes a bit of hard to read:

image

The 'switch' button isn't themed as well.

nuttyartist commented 1 year ago

I'll try to improve the colors and aesthetics later, but thanks for the heads-up. I already deleted the Switch button and replaced it with a shortcut. It's CTRL+SHIFT+K, but maybe you can come up with a better one? I'll add a proper button once I take care of https://github.com/nuttyartist/notes/issues/523.

guihkx commented 1 year ago

Ctrl+Shift+K seems fine to me. :)

We just have to make sure to document it in docs/keyboard_shortcuts.md.

nuttyartist commented 1 year ago

So here are the changes of the latest commit (I hope I didn't break anything with my push, but everything looks fine):

Here's a GIF:

test

nuttyartist commented 1 year ago

@zjeffer @guihkx @bjorn We should test this carefully to make sure there's no data loss as much as we can.

In my testing everything was fine, you should test this as well before we merge (on temporary notes, I would suggest).

zjeffer commented 1 year ago

I think we should change the cursor to a hand when we hover over the grabbable area of the list items. I had a hard time figuring out how to move them because I couldn't find the grabbable area. And during the grab, the cursor should change as well, to the "closed fist" cursor.

And when there are no tasks found, I think we should make that text smaller, and centered in the editor(both horizontally and vertically). Currently it looks like this:

image

Maybe it could look something like this instead:

Untitled

nuttyartist commented 1 year ago

I like both your suggestions, I will implement them. Thanks! Keep it coming.

nuttyartist commented 1 year ago

Can you guys report if the app CPU usage gone up on idle after this PR? I see quite an increase: from 0.0/0.1 to 0.9 when the kanban is hidden. When kanban isn't hidden it's still very high so maybe there's some profiling I need to do.

guihkx commented 1 year ago

Can you guys report if the app CPU usage gone up on idle after this PR?

I'll check that on Linux.

Anyway, with my last two commits I've fixed the macOS build (by invoking macdeployqt with -qmldir).

And I also added an extra step to the CI to run qmllint for Qt 6 builds, just so you can check the warnings easier.

If you're not able to eventually fix all warnings, I can remove it before you merge the PR.

Also, before you finally merge this, I'd like to split some of my changes into smaller commits... But I'll only do that when you feel this PR is ready to merge, just to avoid making you rebase your local branch every time. :P

nuttyartist commented 1 year ago

Can you guys report if the app CPU usage gone up on idle after this PR?

I'll check that on Linux.

Anyway, with my last two commits I've fixed the macOS build (by invoking macdeployqt with -qmldir).

And I also added an extra step to the CI to run qmllint for Qt 6 builds, just so you can check the warnings easier.

If you're not able to eventually fix all warnings, I can remove it before you merge the PR.

Also, before you finally merge this, I'd like to split some of my changes into smaller commits... But I'll only do that when you feel this PR is ready to merge, just to avoid making you rebase your local branch every time. :P

Great, I will let you know.

guihkx commented 1 year ago

Hmm, doesn't this feature work like this anymore?

https://github.com/nuttyartist/notes/assets/626206/158f654d-b9c1-43d7-b4be-12e5c537ede9

:thinking:

nuttyartist commented 1 year ago

Hmm, doesn't this feature work like this anymore?

Video_2023-05-16_12-20-53_edit.mp4

🤔

Weird. It works for me. I'll look into that tomorrow.

nuttyartist commented 1 year ago

I found out the culprit for the high CPU usage using the QML Profiler. I'll fix it.

guihkx commented 1 year ago

Last commit (1ac09272f9dd44851619a3be58681168ded6bf6e) made the font size in popups a little too big on Linux.

Before:

image

After:

image

nuttyartist commented 1 year ago

Last commit (1ac0927) made the font size in popups a little too big on Linux.

Before:

image

After:

image

Yes, you're right. I just tested on Ubuntu and noticed it. I will revert to the previous version. I also think the columns text are too big (I willl switch from point sizes to pixels, I think).

You don't see the previous error you reported (here https://github.com/nuttyartist/notes/pull/574#issuecomment-1549889467) anymore?

guihkx commented 1 year ago

You don't see the previous error you reported (here https://github.com/nuttyartist/notes/pull/574#issuecomment-1549889467) anymore?

I still do, unfortunately. If you want proper steps to reproduce:

  1. Run Notes using the terminal (so you can see the messages)
  2. Create a new note with this content:

    - [ ] task 1
    - [ ] task 2
    - [ ] task 3
    - [ ] task 4
    - [ ] task 5
  3. Hit Ctrl+Shift+K to enter Kanban view

You should now see this warning on the terminal:

<Unknown File>: columnStartLine is undefined. Adding an object with a undefined member does not create a role for it.
qrc:/qt/qml/kanbanMain.qml:367:27: QML Component: Cannot create delegate
qrc:/qt/qml/TodoColumnDelegate.qml:32:5: Required property columnStartLine was not initialized

Additionally, the Kanban view itself still looks 'empty':

image

nuttyartist commented 1 year ago

You don't see the previous error you reported (here #574 (comment)) anymore?

I still do, unfortunately. If you want proper steps to reproduce:

  1. Run Notes using the terminal (so you can see the messages)
  2. Create a new note with this content:
    - [ ] task 1
    - [ ] task 2
    - [ ] task 3
    - [ ] task 4
    - [ ] task 5
  3. Hit Ctrl+Shift+K to enter Kanban view

You should now see this warning on the terminal:

<Unknown File>: columnStartLine is undefined. Adding an object with a undefined member does not create a role for it.
qrc:/qt/qml/kanbanMain.qml:367:27: QML Component: Cannot create delegate
qrc:/qt/qml/TodoColumnDelegate.qml:32:5: Required property columnStartLine was not initialized

Additionally, the Kanban view itself still looks 'empty':

image

Oh odd, I can reproduce it on my Mac as well! I'll get it fixed (: Good catch.

nuttyartist commented 1 year ago

I want to merge this. @guihkx you want to do some squashing?

guihkx commented 1 year ago

@nuttyartist Alrighty. But first, maybe you could sync this PR's branch against master (there are conflicts), and squash some of your recent commits as well?

For features I don't really mind a "one big commit", so feel free to squash everything into that 'Create kanban view based on editor text' commit, and then after I'll split some of my changes.

It'd also buy me a little extra time to properly document the new build flags and dependencies... :sweat_smile:

guihkx commented 1 year ago

@nuttyartist I just pushed the changes I promised:

As usual, I added some more details in the commit's description.

Sorry for the long delay! :S

nuttyartist commented 1 year ago

Awesome work @guihkx. Thanks!