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

Show all url schemas in entry view #1768

Merged
merged 2 commits into from Jul 14, 2018
Merged

Conversation

droidmonkey
Copy link
Member

Description

Fixes #1424

Note: There is still bad behavior in the details view when you have a cmd:// url it will attempt to open a url that is embedded in the command. This is a holdover for icon downloading and can be corrected once icon downloading has the ability to specify arbitrary urls.

How has this been tested?

Manually

Types of changes

  • ✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Mar 22, 2018

Do we want to display the full content of the URL field?

By removing the call to resolveUrl an url like cmd://firefox https://keepassxc.org will display the full command, with resolveUrl instead will return only https://keepassxc.org

The other PR use this second approach #1489

@droidmonkey
Copy link
Member Author

I forgot about that other PR. That one is just a bandaid to the larger problem. We were erroneously filtering and modifying the URL for display (entry->displayUrl()). This one moves us closer to better behavior and cleaner interfaces. As I noted in the description, there still exists some ugly behavior that is dependent on the icon download code.

@tobiasboyd
Copy link

Will this show, for example, ssh://some.example.com along w/ http and https URLs?

@droidmonkey
Copy link
Member Author

@tobiasboyd sorry for the delay, yes this PR displays SSH, HTTP, every url correctly.

}

if (urlString.startsWith("cmd://")) {
if (entry->url().startsWith("cmd://")) {
Copy link
Contributor

@TheZ3ro TheZ3ro Apr 3, 2018

Choose a reason for hiding this comment

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

We should call resolvePlaceholders before this check (like it was done before)

Example scenario:
The current entry has the url set to {REF:A@I:<uuid>} (a reference to another url field)
The other entry has the url field set to cmd://firefox https://github.com

In the current entry this check will be false (but should be true since the referred url does contains cmd://), the url will be treated like a http url and the github.com link will be opened in the default browser (wrong behavior)

In the other entry the url will be correctly treated like a cmd and the execution alert below will be executed

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@droidmonkey droidmonkey changed the base branch from develop to release/2.3.4 June 24, 2018 23:54
@droidmonkey droidmonkey modified the milestones: v2.4.0, v2.3.4 Jun 24, 2018
@droidmonkey
Copy link
Member Author

I moved this to v2.3.4 since it is a relatively minor change

TheZ3ro
TheZ3ro previously requested changes Jul 3, 2018
Copy link
Contributor

@TheZ3ro TheZ3ro left a comment

Choose a reason for hiding this comment

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

There is still my comment about cmd:// url to be fixed.
After that we are ready to merge

@droidmonkey
Copy link
Member Author

I reverted the removal and also fixed an issue with not validating user input when generating UUID's.

@droidmonkey
Copy link
Member Author

ping @TheZ3ro

@droidmonkey droidmonkey merged commit add4ba2 into release/2.3.4 Jul 14, 2018
@droidmonkey droidmonkey deleted the fix/display-url branch July 14, 2018 21:08
droidmonkey added a commit that referenced this pull request Aug 22, 2018
- Show all URL schemes in entry view [#1768]
- Disable merge when database is locked [#1975]
- Fix intermittent crashes with favorite icon downloads [#1980]
- Provide potential crash warning to Qt 5.5.x users [#2211]
- Disable apply button when creating new entry/group to prevent data loss [#2204]
- Allow for 12 hour timeout to lock idle database [#2173]
- Multiple SSH Agent fixes [#1981, #2117]
- Multiple Browser Integration enhancements [#1993, #2003, #2055, #2116, #2159, #2174, #2185]
- Fix browser proxy application not closing properly [#2142]
- Add real names and Patreon supporters to about dialog [#2214]
- Add settings button to toolbar, Donate button, and Report a Bug button to help menu [#2214]
- Enhancements to release-tool to appsign intermediate build products [#2101]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants