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

Improve source management Control #617

Merged
merged 78 commits into from Oct 30, 2020
Merged

Improve source management Control #617

merged 78 commits into from Oct 30, 2020

Conversation

XXXBold
Copy link
Contributor

@XXXBold XXXBold commented Sep 5, 2020

This PR aims to change the source tab to show more information, as suggested in #593 no. 4.
I think some discussion is required in regards of design/implementation details

State:
PR is feature complete, some issue on Mac OS remaining...

First Preview of new source UI:
vorta_srcmgmt_new_draft

@XXXBold
Copy link
Contributor Author

XXXBold commented Sep 6, 2020

Further ideas:

  • Why are there two different buttons to add files or folders? Couldn't that be combined, since multiple selection is possible now?

@samuel-w
Copy link
Contributor

samuel-w commented Sep 6, 2020

Not according to QFileDialog's enums. It's either files or a folder.
https://doc.qt.io/qt-5/qfiledialog.html#FileMode-enum

@m3nu
Copy link
Contributor

m3nu commented Sep 6, 2020

He's right. I already tried that. Can't mix files and folders unless there is a custom file selector class (maybe worth it).

@samuel-w
Copy link
Contributor

samuel-w commented Sep 6, 2020

@XXXBold
Copy link
Contributor Author

XXXBold commented Sep 6, 2020

Okay, didn't know that this isn't possible, so I'll leave this point then.

@m3nu @samuel-w
What do you think about the UI changes? Are you fine with them, or do you have other/more ideas?

@m3nu
Copy link
Contributor

m3nu commented Sep 6, 2020

I think it's great if it works without causing lots of background disk activity. And the calculation needs to happen async in the background. You'll need a placeholder before it's done. If people add network drives it could be even slower.

Also suggest to add an option under Misc to disable it.

@samuel-w
Copy link
Contributor

samuel-w commented Sep 6, 2020

Not UI related, but:
The file/folder checking takes up all of my disk IO, and takes quite a long time. Try using du -sh and du --inodes -s for faster folder size/file count.
Both are standard Unix, so shouldn't be an issue.

@XXXBold
Copy link
Contributor Author

XXXBold commented Sep 6, 2020

It doesn't cause disk activity in the background (Well, it does once, when it calculates the sizie initial).
The placholder already exists (just the text "load..." for now), and it's processed in the background (1 thread/path), once when a source is added or when vorta is started.

An additional button to manually update the sizes could be placed somewhere...

@m3nu
Copy link
Contributor

m3nu commented Sep 6, 2020

Yeah. That will be the issue. If this runs whenever the profile changes, it will cause heavy disk activity.

@XXXBold
Copy link
Contributor Author

XXXBold commented Sep 6, 2020

Not UI related, but:
The file/folder checking takes up all of my disk IO, and takes quite a long time. Try using du -sh and du --inodes -s for faster folder size/file count.
Both are standard Unix, so shouldn't be an issue.

Hm, are you sure you want to go the unix way? I sticked to QT for portability (Future windows version, for example), but I'm up for discussion. Also, currently there's an artificial delay for 5 seconds (for testing) before each thread returns.

@m3nu
Copy link
Contributor

m3nu commented Sep 6, 2020

I would only calculate when adding or when it's manually refreshed. Whenever it's opened will be too often. E.g. I have multiple profiles to do different home folder backups. So it would size my whole home folder multiple times?

Also: how about Borg exclusions below? Once I exclude large folders, the calculated total will be meaningless. :-(

This is a hard problem.

@samuel-w
Copy link
Contributor

samuel-w commented Sep 6, 2020

Maybe calculate the exclusion size separately and subtract? Would lead to double the IO worst case.

@XXXBold
Copy link
Contributor Author

XXXBold commented Sep 6, 2020

I didn't think about exclusions for now tbh, since the idea was to only show the current size of each entry, but true, that's an issue aswell.

If you don't want the size to be calculated on startup, it needs to be saved in the database, along with the path I guess?

@samuel-w
Copy link
Contributor

samuel-w commented Sep 6, 2020

Yeah, should be part of SourceFileModel

@m3nu
Copy link
Contributor

m3nu commented Sep 6, 2020

Yeah, should be part of SourceFileModel

Yes. Good idea. We don't have much in that table yet. At least it will be used for something. So I'd only calculate the size when adding it or manually refreshing. Together with a checkbox in Misc to disable calculating it when adding. E.g. "Calculate source folder statistics when adding." Enabled by default.

@XXXBold
Copy link
Contributor Author

XXXBold commented Sep 6, 2020

Okay, I'll do some reworks. BTW what's the best practice in python for interfacing with native APIs (the mentioned du command for example)? Running as a cmd and parse the output string? Or is there a better method?

@ghost
Copy link

ghost commented Sep 6, 2020

@XXXBold Marginal observation, which I would like to share when I look at the screenshot above:
Add Folder -> singular
Add File(s) -> singular + plural
Paste Folders/Files -> singular + plural but both forms written out.
-> Would it be possible to make the wording more consistent here?

@m3nu
Copy link
Contributor

m3nu commented Sep 6, 2020

Okay, I'll do some reworks. BTW what's the best practice in python for interfacing with native APIs (the mentioned du command for example)? Running as a cmd and parse the output string? Or is there a better method?

If a lot of work is done by the CLI tool and the CLI tool has no mode for json (or similar) structured output, then calling it from subprocess can be surprisingly efficient.

E.g. I used to size folders in Python, but also use du now and didn't have issues. Just note that the args may be different between macOS and Linux. For Linux I use:

def get_folder_size(path):
    res = subprocess.run('du -sb .', cwd=path, shell=True, check=False,
                         stdout=subprocess.PIPE, stderr=subprocess.DEVNULL)
    return int(res.stdout.split(b'\t')[0])

macOS is most likely just du -s. (need to be careful with block sizes, apparent size)

Not working yet, requires a new DB version I guess.
Would appreciate any help in this regard
@XXXBold
Copy link
Contributor Author

XXXBold commented Sep 6, 2020

I've added new attributes for the SourceFileModel. However, I think this requires a new DB-Version? Would appreciate if someone could help me with this, no idea what to do there :P

Also, if this changes anyway, should the "dir" attribute be renamed to "dirPath"? Would just be more obvious then.

@samuel-w
Copy link
Contributor

samuel-w commented Sep 6, 2020

You need to add schema upgrading code in init_db, for this:

    if current_schema.version < 16:
        _apply_schema_update(
            current_schema, 16,
            migrator.add_column(SourceFileModel._meta.table_name,
                                'dirSize', pw.IntegerField()),
            migrator.add_column(SourceFileModel._meta.table_name,
                                'dirFilesCount', pw.IntegerField()),
            migrator.add_column(SourceFileModel._meta.table_name,
                                'dirType', pw.BooleanField())
        )

Along with bumping SCHEMA_VERSION
Also multi word variables are in formatted as snake_case not camelCase in Python.

@XXXBold
Copy link
Contributor Author

XXXBold commented Sep 6, 2020

Thanks! I've also considered your hint for the naming convention, and changed the variable names, will also look over the other files...

XXXBold and others added 6 commits October 14, 2020 18:51
Co-authored-by: Samuel <samuel.woon@protonmail.com>
Co-authored-by: Samuel <samuel.woon@protonmail.com>
Co-authored-by: Samuel <samuel.woon@protonmail.com>
Co-authored-by: Samuel <samuel.woon@protonmail.com>
@XXXBold
Copy link
Contributor Author

XXXBold commented Oct 14, 2020

All changes made/approved as requested.
Now, only the python-method is used to get the folder size/files count.
For me, the PR is now feature complete. Thanks once again @samuel-w and @m3nu for the great assistance on various topics!

@m3nu
Copy link
Contributor

m3nu commented Oct 15, 2020

Thanks for the contribution, @XXXBold . Great to see everyone tackling a problem to arrive at the best solution.

Let's proceed to review and then merge.

@m3nu m3nu requested review from m3nu and samuel-w October 15, 2020 01:16
@m3nu
Copy link
Contributor

m3nu commented Oct 17, 2020

Just tried this locally. Works well and not intrusive, if someone doesn't want/need to calculate the sizes. Some thoughts I had:

  • The table could use rows with different (striped) background colors, as we already do in the Archive tab. self.archiveTable.setAlternatingRowColors(True)
  • Would Qt support resizing the source table, while making the exclude area smaller?

And for future improvements:

  • We could consider using multiple horizontal "Toolbox" screens on the Source tab, like we do for Archives and Schedule. To give enough space to the source table and re-arrange the buttons like we have on the Archives tab. Then it would also easier to expand the different Exclude options. E.g. manage exclusions in a better way than just lines in a textbox. See also Suggestion: Exclude patterns GUI improvements #399 (comment)

Copy link
Contributor

@samuel-w samuel-w left a comment

Choose a reason for hiding this comment

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

Functionality is fine, just some minor style changes.

src/vorta/models.py Outdated Show resolved Hide resolved
src/vorta/utils.py Outdated Show resolved Hide resolved
src/vorta/utils.py Outdated Show resolved Hide resolved
@samuel-w
Copy link
Contributor

For the alternating colours, IMO it looks cleaner with the same colour throughout.

XXXBold and others added 3 commits October 20, 2020 17:46
Co-authored-by: Samuel <samuel.woon@protonmail.com>
Co-authored-by: Samuel <samuel.woon@protonmail.com>
Co-authored-by: Samuel <samuel.woon@protonmail.com>
src/vorta/utils.py Outdated Show resolved Hide resolved
Co-authored-by: Samuel <samuel.woon@protonmail.com>
Copy link
Contributor

@samuel-w samuel-w left a comment

Choose a reason for hiding this comment

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

Now that I think about it, Decimal is not really needed.

src/vorta/utils.py Outdated Show resolved Hide resolved
src/vorta/utils.py Outdated Show resolved Hide resolved
XXXBold and others added 2 commits October 26, 2020 14:47
Co-authored-by: Samuel <samuel.woon@protonmail.com>
Co-authored-by: Samuel <samuel.woon@protonmail.com>
@m3nu
Copy link
Contributor

m3nu commented Oct 30, 2020

What's the status here? Are we good to merge? Since this is a larger PR, would be good to get it merged before we get conflicts with other PRs.

@m3nu m3nu self-assigned this Oct 30, 2020
Copy link
Contributor

@samuel-w samuel-w left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Tested:

  • Empty folder (Size 0B File Count 0)
  • Folder with one file (Size 2B File Count 1)

Might want to add a note on the sources tab about the size reducing with compression.
Later we could use the exclude patterns and exclude if present to get a more accurate total size.

@m3nu
Copy link
Contributor

m3nu commented Oct 30, 2020

Works for me as well. Thanks for the great contribution, @XXXBold !

Might want to add a note on the sources tab about the size reducing with compression.
Later we could use the exclude patterns and exclude if present to get a more accurate total size.

Let's save those for later, when we re-arrange the whole Source tab to use multiple toolbox areas (source folders, exclusions). The PR is already large enough.

@m3nu m3nu merged commit 461ea05 into borgbase:master Oct 30, 2020
@m3nu m3nu mentioned this pull request Oct 31, 2020
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.

None yet

3 participants