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

LogViewDialog: Remove hidden feature "Exclude" because it is redundant #1578

Closed
buhtz opened this issue Dec 7, 2023 · 6 comments · Fixed by #1695
Closed

LogViewDialog: Remove hidden feature "Exclude" because it is redundant #1578

buhtz opened this issue Dec 7, 2023 · 6 comments · Fixed by #1695
Assignees
Labels
Discussion decision or consensus needed GOOD FIRST ISSUE Used by 24pullrequests.com to suggest issues HELP-WANTED Used by 24pullrequests.com to suggest issues Medium Qt Qt bugs, code or features

Comments

@buhtz
Copy link
Member

buhtz commented Dec 7, 2023

Accidentally I discovered a feature in the class LogViewDialog that is kind of hidden. Screencasts say more then words:

Peek 2023-12-07 16-03

I vote to remove the "Exclude" and the "Copy" feature from this dialog because:

  • The "Copy" works out of the box in each Qt editable text element. No need to overwrite it with an own Copy implementation.
  • The "Exclude" is kind of hidden or unusual to access. I assume that there is nearly no one excluding files like this.
  • Excluding files that are currently in the snapshot can be easily done in the main window.

It will reduce code complexity.

def contextMenuClicked(self, point):
menu = QMenu()
clipboard = qttools.createQApplication().clipboard()
cursor = self.txtLogView.textCursor()
btnCopy = menu.addAction(_('Copy'))
btnCopy.triggered.connect(lambda: clipboard.setText(cursor.selectedText()))
btnCopy.setEnabled(cursor.hasSelection())
btnAddExclude = menu.addAction(_('Add to Exclude'))
btnAddExclude.triggered.connect(self.btnAddExcludeClicked)
btnAddExclude.setEnabled(cursor.hasSelection())
btnDecode = menu.addAction(_('Decode'))
btnDecode.triggered.connect(self.btnDecodeClicked)
btnDecode.setEnabled(cursor.hasSelection())
btnDecode.setVisible(self.config.snapshotsMode() == 'ssh_encfs')
menu.exec_(self.txtLogView.mapToGlobal(point))
def btnAddExcludeClicked(self):

EDIT: Asked on reddit for fresh contributors because this seems to be GOOD FIRST ISSUE.

@buhtz buhtz added Discussion decision or consensus needed Medium GOOD FIRST ISSUE Used by 24pullrequests.com to suggest issues HELP-WANTED Used by 24pullrequests.com to suggest issues labels Dec 7, 2023
@buhtz buhtz added this to the 2rd release from now milestone Dec 7, 2023
@buhtz buhtz added the Qt Qt bugs, code or features label Dec 7, 2023
@emtiu
Copy link
Member

emtiu commented Dec 7, 2023

I agree with your assessment, good catch!

@aryoda
Copy link
Contributor

aryoda commented Dec 7, 2023

I love "easter eggs", why remove it (does it hurt?)?

Edit: We could also add short label at the bottom of the dialog mentioning the context menu...

BTW: Could we use this context menu to add a context-sensitive help for the 11-char rsync itemize-changes option (see my FR #1579)?

@buhtz
Copy link
Member Author

buhtz commented Dec 8, 2023

image

This is an example how I imagine a log view. With a widget like this it is more easy to use tool tips and all the other fancy and usability-improving this of a GUI. But we do not have resources to implement it and our priorities are different from heavy GUI improvements like this.

I love "easter eggs", why remove it (does it hurt?)?
😄 Unused translatable strings (that is how I discovered this), untested code, decreased coverage, etc pp. You know me. I like it clean and clear.

Edit: We could also add short label at the bottom of the dialog mentioning the context menu...
From an UI design perspective it feels like a workaround.

The whole feature is creepy. It has a high risk of introducing problems by the users because the user need to highlight the right text. If the text is not correct (and we do not check it) the behavior is unpredictable.

BTW: Could we use this context menu to add a context-sensitive help for the 11-char rsync itemize-changes option (see my FR #1579)?
Not sure but I assume it is not possible the easy way to get correct location of the mouse cursor and its corresponding text under it. It is just a multi-line text-field. It is much easier when having a nice widget like I show in the example picture.

@aryoda
Copy link
Contributor

aryoda commented Dec 8, 2023

This is an example how I imagine a log view. With a widget like this it is more easy to use tool tips and all the other fancy and usability-improving this of a GUI.

Yes, a structured grid view would be nice and also challenging since the log output is semi-structured needs robust parsing, see eg. this typical output (filter = all):

========== Take snapshot (profile 1): Wed Dec  6 11:48:10 2023 ==========

[I] ...
[I] Taking snapshot
[I] rsync --recursive --times --devices --specials --hard-links --human-readable -s --copy-links --acls --perms --executability --group --owner --info=progress2 --no-inc-recursive --delete --delete-excluded -v -i --out-format=BACKINTIME: %i %n%L --link-dest=../../20231206-114635-194/backup --chmod=Du+wx --exclude=/home/JaneDoe/temp/testBAK_profil1 --exclude=/home/JaneDoe/.local/share/backintime --exclude=.local/share/backintime/mnt --include=/home/JaneDoe/Documents/ --include=/home/JaneDoe/ --include=/home/ --include=/home/JaneDoe/temp/deleted_folder/ --include=/home/JaneDoe/temp/ --include=/home/JaneDoe/.mozilla/ --exclude=.gvfs --exclude=.cache/* --exclude=.thumbnails* --exclude=.local/share/[Tt]rash* --exclude=*.backup* --exclude=*~ --exclude=.dropbox* --exclude=/proc/* --exclude=/sys/* --exclude=/dev/* --exclude=/run/* --exclude=/etc/mtab --exclude=/var/cache/apt/archives/*.deb --exclude=lost+found/* --exclude=/tmp/* --exclude=/var/tmp/* --exclude=/var/backups/* --exclude=.Private --exclude=lock --include=/home/JaneDoe/Documents/** --include=/home/JaneDoe/temp/deleted_folder/** --include=/home/JaneDoe/.mozilla/** --exclude=* / /home/JaneDoe/temp/testBAK_profil1/backintime/PC1/JaneDoe/1/new_snapshot/backup

[I] Take snapshot (rsync: building file list ... done)
[I] Take snapshot (rsync: BACKINTIME: .d..tp..... ./)
[I] Take snapshot (rsync: sent 1,68M bytes  received 19 bytes  479,80K bytes/sec)
[I] Take snapshot (rsync: total size is 1,22G  speedup is 723,91)
[I] 'rsync' ended with exit code 0: Success
[I] Saving config file…
[I] Saving permissions…

The whole feature is creepy. It has a high risk of introducing problems by the users because the user need to highlight the right text. If the text is not correct (and we do not check it) the behavior is unpredictable.

I fully agree with that so let's remove this functionality

@Sakh1l
Copy link

Sakh1l commented Dec 9, 2023

Can this #1578 be assigned to me?

@buhtz
Copy link
Member Author

buhtz commented Dec 9, 2023

May I introduce @Sakh1l to the others here. He contacted me via reddit after my good-first-issue-announcment there.

Can this #1578 be assigned to me?

Done.
Have a look at https://github.com/bit-team/backintime/blob/dev/CONTRIBUTING.md and let me know if I can help you somehow. Also feel free to contact me via email.

buhtz added a commit that referenced this issue Apr 28, 2024
The context menu in the LogViewDialog is removed without replacement.

- "Copy" is offered by Qt default context menu in this widget.
- Translate to English: The function "Exclude" was not user-friendly and prone to errors.
- "Decode" (only in SSH encrypted profiles) is still accessible via a check box in the same dialog window.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion decision or consensus needed GOOD FIRST ISSUE Used by 24pullrequests.com to suggest issues HELP-WANTED Used by 24pullrequests.com to suggest issues Medium Qt Qt bugs, code or features
Projects
None yet
4 participants