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

Allow Prefilling Add Server Dialog With HTTP URLs #3182

Merged
merged 12 commits into from Nov 11, 2017

Conversation

@Kissaki
Copy link
Member

commented Jul 17, 2017

The commits in this PR restructure the ConnectDialogEdit code and implement suggesting to the user to fill the dialog form from the clipboard (if clipboard data is available). Implements #3143


Outdated

2017-07-18 01_42_33-add server

TODO:

  • Use existing themeable color/style
  • Fix/Need layout auto-adjustment on show/hide
  • Implement fill suggestion for current connected to server (fallback if no clipboard data)
  • Prevent vertical resizability

@mkrautz @davidebeatrici @hacst what do you think about this new UI element (as per screenshot)?

Also makes sense for prefilling with the current connected to server instead of autofilling like we currently do, right? (Even though we can not have invalid/wrong data there like for HTTP URLs.)

@Kissaki Kissaki added mumble ui labels Jul 17, 2017

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jul 18, 2017

It's beautiful.

Try to change the background color to grey or cyan and see if it's better.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jul 18, 2017

I'd like a button for dismissing the notice.
Also, the button should maybe say "Fill" instead?

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jul 18, 2017

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2017

Okay,thanks. I’ll polish this then.

@klemun

This comment has been minimized.

Copy link

commented Jul 18, 2017

I think 'You have an URL...' should be 'You have a URL...'. I'm also working on a mock-up of this window in Illustrator just to see how it could look with different colors.

Dark theme:
dark

Lighter themes:
light

Can change anything on these if you wish to see or if you have Adobe Illustrator yourself I can upload the file somewhere.

Kissaki added 5 commits Jul 12, 2017
ConnectDialog: Separate fromMimeData and fromUrl
Introduce new method fromUrl.

fromMimeData: mime to mumble URL, calls fromUrl

fromUrl: mumble URL to ServerItem
ConnectDialog: Move ConnectDialogEdit prefill logic into separate con…
…structor

ConnectDialogEdit now has two constructors. One for editing a server,
and one for adding a new (favorite) one. Both use the new init method
for common initialization.

Drop logic for bonjour name as ServerItem::fromMimeData never sets a
bonjour address.

Delete ServerItem after use; must have been a memory leak before.

Slightly change logic of default username; always set it if empty
(also for servers from the clipboard).
ConnectDialog: Move default server name logic to fromMimeData
fromUrl gets passed a finished mumble:// URL with adequate title parameter

@Kissaki Kissaki force-pushed the Kissaki:3143-http-urls branch from b4b70ac to 989a10b Aug 6, 2017

@Kissaki Kissaki changed the title WIP: Allow Prefilling Add Server Dialog With HTTP URLs Allow Prefilling Add Server Dialog With HTTP URLs Aug 6, 2017

@Kissaki Kissaki changed the title Allow Prefilling Add Server Dialog With HTTP URLs WIP: Allow Prefilling Add Server Dialog With HTTP URLs Aug 6, 2017

@Kissaki Kissaki changed the title WIP: Allow Prefilling Add Server Dialog With HTTP URLs Allow Prefilling Add Server Dialog With HTTP URLs Aug 6, 2017

@Kissaki Kissaki requested review from mkrautz, hacst and davidebeatrici Aug 6, 2017

@@ -155,5 +155,5 @@ bool Themes::readStylesheet(const QString &stylesheetFn, QString &stylesheetCont
}

QString Themes::getDefaultStylesheet() {
return QLatin1String(".log-channel{text-decoration:none;}.log-user{text-decoration:none;}p{margin:0;}#qwMacWarning{background-color:#FFFEDC;border-radius:5px;border:1px solid #B5B59E;}");
return QLatin1String(".log-channel{text-decoration:none;}.log-user{text-decoration:none;}p{margin:0;}#qwMacWarning,#qwInlineNotice{background-color:#FFFEDC;border-radius:5px;border:1px solid #B5B59E;}");

This comment has been minimized.

Copy link
@mkrautz

mkrautz Aug 6, 2017

Member

I guess we need a PR in mumble-theme as well?

This comment has been minimized.

Copy link
@Kissaki

Kissaki Aug 6, 2017

Author Member

Yes

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2017

Ready for review.

I did not manage to make the dialog not resize vertically when the notice is hidden. It works when it shows. Before these changes the dialog was also vertically resizable with no use, so at least it's not worse.

Theme changes to dark and lite will have to be committed separately to the theme repo.

Screenshots of both cases and all three skins follow.

image
image
image
image

@mkrautz

This comment has been minimized.

Copy link
Member

commented Aug 6, 2017

I only did a cursory glance. PR looks great. Easy to review with the split commits. Great job.
I will do a thorough review ASAP, but not now at 00:30. :-)

Quick comments on the layout of the notice:

  • I think the X button should be "Ignore" (or maybe "Dismiss", but "Ignore" is a simpler word in my mind)
  • Can we stack the buttons vertically instead? (row layout?)

@Kissaki Kissaki force-pushed the Kissaki:3143-http-urls branch from 64682a3 to 779e658 Aug 7, 2017

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2017

Labeled the buttons with text and icons (dingbats, unicode characters). Also added a newline to the connected server to match the other text and I think it looks better.
image

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2017

Tried to align them right, but that didn’t work… 🤔

ConnectDialog: Handle HTTP URLs from clipboard
Implements feature request #3143

In various communication programs when a user writes a Mumble server
address without the mumble:// schema, the program will interpret a
domain example.com as a HTTP hyperlink and convert it to a HTTP
hyperlink. Some programs will only make the text as is a hyperlink,
others will also convert the written text to include http (which will
make it more confusing for users who are not familiar with HTTP, URLs
and schemas/the Mumble schema).

Instead of pre-filling the add server dialog, introduce a
paste-able-clipboard notice to the ConnectDialogEdit that suggests to
the user to pre-fill with the clipboard content.

@Kissaki Kissaki force-pushed the Kissaki:3143-http-urls branch from 779e658 to c90b36f Aug 7, 2017

@Kissaki Kissaki force-pushed the Kissaki:3143-http-urls branch from c90b36f to cea7064 Aug 7, 2017

ConnectDialogEdit: layout updates.
Ensure the paste notice label is set to expand vertically, and align
its text at the top.

Promote the qlActions layout to a QWidget, and ensure its margins are
set to 0. Its new name is qwActions.

Apply a horizontal stretch of 4 to the qlPasteNotice label, and a
horizontal stretch of 1 to the qwActions container.

Set the horizontal size policy of both qbDisard and qbFill to be
Expanding, ensuring that the buttons have the same size.

Remove checkmark and cross glyphs from the qbDiscard and qbFill
buttons. They aren't the same size on Windows, which looks odd.
Also, using such glyphs in buttons is not something we usually do,
so I would rather be without it.
@mkrautz

This comment has been minimized.

Copy link
Member

commented Aug 12, 2017

I have made some adjustments to the layout of the dialog:

image
image

Commit at mkrautz@9709ed8
Feel free to integrate it. (Maybe squash it into the original?)

@@ -214,6 +214,7 @@ class ConnectDialogEdit : public QDialog, protected Ui::ConnectDialogEdit {
void on_qcbShowPassword_toggled(bool);
void on_qleName_textEdited(const QString&);
void on_qleServer_textEdited(const QString&);
void showNotice(const QString & text);

This comment has been minimized.

Copy link
@mkrautz

mkrautz Aug 12, 2017

Member

Coding style: & should hug text.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Aug 12, 2017

Also, I think auto-updating from the clipboard might be surprising...

When I was making the screenshots for my post above, I used Alt+PrintScreen. This caused the notice to disappear because my clipboard was updated with a screenshot.

So, one could argue that it should only refresh its state if the clipboard contains a valid item for the connect dialog.

But even then... I am not sure if automatically syncing from the clipboard should be there. Consider a user that is multitasking. It wouldn't be out of the realm of possibility for a user to open the add server dialog and get distracted. When the user returns to the dialog, the system's clipboard content might now contain something else, or an actual web URL, or whatever. Since the dialog updates automatically, the user might not notice. So I actually think we should back off from this.

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2017

Picked, and fixed format.

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2017

Is the Travis apple build broken?

Info: creating stash file /Users/travis/build/mumble-voip/mumble/.qmake.stash
Cannot read /Users/travis/build/mumble-voip/mumble/scripts/scripts.pro: No such file or directory
…
    #include <Ice/SliceChecksumDict.ice>
1 error in preprocessor.

Retriggering did not help.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Sep 2, 2017

Yes, I don't know what's wrong yet.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Sep 2, 2017

I guess the paths for Homebrew's Ice changed. Probably due to Ice 3.7.

Do not update fill suggestion on clipboard changes
This means the user can not copy a Mumble URL while the dialog is open
and fill in the data from it. Instead, he will have to close and reopen
the dialog.

This change is to prevent potential confusion in case the user reads the
fill suggestion, (briefly) switches tasks, and then has different data
in their clipboard. This would also change the suggested fill data,
potentially without the user noticing the change, and then confusion
about the filled data.
@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Nov 11, 2017

Regarding the vertical sizing: Before these changes, the dialog can be resized vertically as well, leading to increasing space between the form rows. So this PR does not make it worse.

After changing vsizetype="MinimumExpanding" to vsizetype="Fixed" on the dialog I would have expected it to be unchangeable vertically, which is not the case. Only after adding a sizeConstraint on the layout (QLayout::SetMinAndMaxSize`) is it only marginally resizable vertically.
Just now I set vertical to Fixed for every control, and it has the same effect but only with the notice open/displayed. Without it, it still expands.

So rather than change anything here I don't understand, I'd rather keep it to allow vertical resizing - for now, and not touch that in this PR.

@davidebeatrici davidebeatrici merged commit 3eae0dc into mumble-voip:master Nov 11, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Kissaki Kissaki deleted the Kissaki:3143-http-urls branch Nov 11, 2017

Kissaki added a commit that referenced this pull request Nov 11, 2017
2uh pushed a commit to 2uh/Mutter that referenced this pull request Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.