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

Add full path in failed files modal #4023

Closed
wants to merge 2 commits into from

Conversation

nov1n
Copy link
Contributor

@nov1n nov1n commented Mar 6, 2017

Purpose

Display the full file path in the failed files modal such that a user may copy the path in order to investigate.

Testing

Tested on Chrome and Firefox on Windows and Ubuntu.

Screenshots

image

@nov1n nov1n mentioned this pull request Mar 6, 2017
@calmh
Copy link
Member

calmh commented Mar 6, 2017 via email

@nov1n
Copy link
Contributor Author

nov1n commented Mar 6, 2017

The tooltip wraps but the actual text does not:

image

We can make this horizontally scrollable but I think this is a bad user experience. Wrapping filepaths with word-wrap:break-word looks like this which may be acceptable:

image

What do you think?

@calmh
Copy link
Member

calmh commented Mar 6, 2017 via email

@nov1n
Copy link
Contributor Author

nov1n commented Mar 9, 2017

@calmh I updated the PR andI swapped out the table-condensed for a new table-dynamic class which solves the problems.

The columns dynamically scale:
image

Long lines wrap if there is no space left:
image

@AudriusButkevicius
Copy link
Member

Can you add yourself to authors/nicks/aliases whatever?

Otherwise, this looks good to me.

@calmh
Copy link
Member

calmh commented Mar 18, 2017 via email

@calmh
Copy link
Member

calmh commented Mar 18, 2017

Done.

@st-jenkins retest this please

@calmh
Copy link
Member

calmh commented Mar 18, 2017

I think the table should still be table-condensed even if dynamic, but I'm not going to fight over it.

@calmh
Copy link
Member

calmh commented Mar 18, 2017

@st-review lgtm

@st-review
Copy link

@calmh: Noted! Need another LGTM or explicit merge command.

@AudriusButkevicius
Copy link
Member

@st-review lgtm

@st-review
Copy link

@AudriusButkevicius: Merge failed:

$ git fetch -f origin refs/pull/4023/head:pr-4023
$ git fetch -f origin master:orig/master
From github.com:syncthing/syncthing
   88dfd63..c6dd777  master     -> orig/master
   1caa683..c6dd777  master     -> origin/master
$ git reset --hard
HEAD is now at 1caa683 lib/scanner: Stopped outputting rescan debug message if file doesn't exist locally (fixes #1350)
$ git checkout master
Already on 'master'
Your branch is behind 'origin/master' by 1 commit, and can be fast-forwarded.
  (use "git pull" to update your local branch)
$ git reset --hard orig/master
HEAD is now at c6dd777 authors: Add nov1n
$ git clean -fxd
$ git merge --squash --no-commit pr-4023
Automatic merge went well; stopped before committing as requested
Squash commit -- not updating HEAD
$ git commit -F -
Commit message subject:

    Add full path in failed files modal

doesn't look like "tag: One sentence description". Specifically, it doesn't
match this pattern:

    ^[\w/,\. ]+: \w

@AudriusButkevicius
Copy link
Member

@st-review merge

gui: Show full failed item path

@st-review
Copy link

👌 Merged as ed771f5. Thanks, @nov1n!

@st-review st-review closed this Mar 18, 2017
st-review pushed a commit that referenced this pull request Mar 18, 2017
@st-review st-review added the pr-merged Legacy label used in the past for pull requests merged to the main tree label Jan 15, 2018
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Jul 15, 2018
@syncthing syncthing locked and limited conversation to collaborators Jul 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion pr-merged Legacy label used in the past for pull requests merged to the main tree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants