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

default zoom factor for webbrowser is configurable in settings #89

Merged
merged 12 commits into from Dec 21, 2018

Conversation

Projects
None yet
5 participants
@balcy
Copy link
Collaborator

balcy commented Nov 6, 2018

add new page menu for:

  • setting the current zoom factor for the view
  • save current page as '.mhtml'
  • save current page as PDF
  • view source of current page

fixes #95

@balcy balcy referenced this pull request Nov 9, 2018

Closed

print to pdf function #95

@balcy

This comment has been minimized.

Copy link
Collaborator

balcy commented Nov 10, 2018

the log says src/app/webbrowser/morph-browser.cpp:178:36: error: 'AA_EnableHighDpiScaling' is not a member of 'Qt' QCoreApplication::setAttribute(Qt::AA_EnableHighDpiScaling); (the file is not changed with that PR)
and there is -rdynamic /usr/lib/arm-linux-gnueabihf/libQt5Core.so.5.5.1 in the log, isn't that a too old Qt version ?

@balcy balcy requested review from mariogrip and nfsprodriver Nov 10, 2018

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

UniversalSuperBox commented Nov 12, 2018

Whoa! Now this is cool.

This PR can be installed on your device running Ubuntu Touch 16.04 from the devel or rc channel with the following command:

sudo ubports-qa install morph-browser 89

After that, restart morph-browser if it's running. There is now a "Page Menu" option in the overflow menu which you can use.

@cibersheep, can you comment on the look and feel of this menu and its options?

Some things I notice just from toying around:

  • PDFs are saved to Downloads, but the Document Viewer can only open files from Documents by itself. Otherwise the files need to be opened through the file manager or Downloads from the browser. Not terrible, just an observation.
  • It's not terribly obvious how to leave "View Source" view. I saw after a bit of toying that the page is opened in a separate tab, but I'd prefer a "close" button somewhere.
  • The default page zoom takes effect only in tabs created after the setting is set. That should be communicated somewhere.
  • It's difficult to get page zoom back to 100% after changing it.
  • The tooltip telling you the current page zoom level appears behind the Set Desktop mode list item.
@balcy

This comment has been minimized.

Copy link
Collaborator

balcy commented Nov 12, 2018

thank you for the detailed review !

  • for the first point: The advantage of using downloads was that the "Save as html" file uses downloads automatically, and that you can choose "open with" from downloads easily. If morph has access to the documents folder we could certainly save the PDFs there as well.
  • the second item is discussed in #50 (close current tab)
  • 3rd point: (zoom does not take effect) I have to look into that, I think I have removed code that I already had for that, because I thought it was not needed :)
  • [4] is valid as well, there should be a reset possibility I think, do you think it would be improved by letting the user only set 10% steps (instead of 5%) ?
  • [5] agreed (on BQ Aquaris 4.5 I see half of it), I've found not way yet to improve that, does s.o. have an idea ?
@cibersheep

This comment has been minimized.

Copy link

cibersheep commented Nov 13, 2018

@cibersheep, can you comment on the look and feel of this menu and its options?

I think tomorrow I'll be able to test that

@balcy

This comment has been minimized.

Copy link
Collaborator

balcy commented Nov 13, 2018

[3] should be gone now (default zoom change in settings is applied to current page)
[4] I've included a reset icon
[5] still no luck with that

@cibersheep

This comment has been minimized.

Copy link

cibersheep commented Nov 15, 2018

I have tested a couple of things and I must say is a cool feature congratulations.
The menu itself I feel it a bit confusing.

We have all this menus

imatge

imatge

imatge

Thinking that the zoom menu is going to be used only in one page, I suggest to replace the PageHeader with it (like we do in a page search). That will us free from placement problems and it will follow a bit of the system philosophy.

We could do that with the selection menu also, as if we select the text in the address bar we get out old copy / paste menu.

I don't know what to do with the image popup though.

What do you think?

@balcy

This comment has been minimized.

Copy link
Collaborator

balcy commented Nov 15, 2018

Thank you for testing !
I think you're right that it could be confusing having some items in a menu that is at another place.
What about that:
Add the following items to the main menu:
"Save as HTML/PDF" -> opens a dialog where the user can click "Save as HTML", "Save as PDF", "Cancel"
"View Source" -> opens source in new tab
"Zoom" -> opens zoom settings for the page
Those could look like the current page menu (but only with "Zoom Out", "Zoom In" and "Cancel")

Instead of placing it top right, I could always make it show up at the center position (like the select all context menu, and the link / image context menu)

The page header option would mean, that we can no longer use the "searchmode" property for the address bar.
The zoom for the current page (like the shortcuts CTRL+Plus, CTRL+Minus) should be available not only for the morph browser, but for webapps too. (those do not generally have the header)

For the context menu itself I think it is important that it stays close to the (input) element it was created for, especially on the phone.

@cibersheep

This comment has been minimized.

Copy link

cibersheep commented Nov 15, 2018

I see. Quite good points there.

Even I think the zoom and search at the same time could be rare, is a valid point, also the webapp without header is a total «yes, you're right».

Putting the save/export and zoom into the menu looks better but if the webapp has no main menu, then the zoom feature is not accessible :) only the pinch. Would it be too cumbersome to show the zoom menu on the pinch gesture? (I'm thinking out loud here) A different gesture for the zoom would be just too much I think (two fingers up/down?)

All this leads as to menu: [Select less, Select Element, Select All, Share Text, Zoom, Cancel] ? As a vertical menu?? (I'm not sure at all about this, copy/paste text menu in a TextField is horizontal but contextual menu on a link/image is vertical)

About the zoom menu: centred would look better and it could be [reset zoom, zoom out, zoom%, zoom in, cancel]

Does any of this makes any sense?

@balcy balcy force-pushed the balcy:xenial_-_zoomcontrol branch from 9d19c76 to f095709 Nov 16, 2018

@balcy

This comment has been minimized.

Copy link
Collaborator

balcy commented Nov 16, 2018

Yes I think it your thoughts make sense : )
I have to say I like the suggestion with putting current zoom to the page header as well. (then we would have a standard mode, a search mode, and a "zoom setting" mode)
Maybe for webapps it is not even needed (or it can be handled differently there)

The pinch gesture would be good to connect the zoom with, but I've not found out how I can get the event...
If pinch is used in morph, I currently have trouble with placing the context menu for the input fields.
The variables going into this position is the current scroll position, the scale factor and the browser zoom. The pinch adds another property here that I would (if possible) eliminate. So if the pinch could used for zooming it would be great, but currently I don't know a way.

All this leads as to menu: [Select less, Select Element, Select All, Share Text, Zoom, Cancel] ? ... My compromise for this was only showing the icon to the menu, if no text was selected (long press on an item in the page without text, like in your first screenshot)

On the phone the link menu looks much bigger (almost full screen) compared to the context menu, and the context menu tries to stay next to the long pressed element
contextmenu_linkmenu

@cibersheep

This comment has been minimized.

Copy link

cibersheep commented Nov 16, 2018

I have to say I like the suggestion with putting current zoom to the page header as well. (then we would have a standard mode, a search mode, and a "zoom setting" mode)
Maybe for webapps it is not even needed (or it can be handled differently there)

I could do a mockup if you like

The pinch gesture would be good to connect the zoom with, but I've not found out how I can get the event...

The only think I have found on the subject is this change:
1439e67

All this leads as to menu: [Select less, Select Element, Select All, Share Text, Zoom, Cancel] ?
... My compromise for this was only showing the icon to the menu, if no text was selected (long press on an item in the page without text, like in your first screenshot)

This could be used to show the zoom menu though

On the phone the link menu looks much bigger (almost full screen) compared to the context menu, and the context menu tries to stay next to the long pressed element

I see. Then it makes sense to be horizontal and close to the selection :) no point to select something you can't see

@balcy

This comment has been minimized.

Copy link
Collaborator

balcy commented Nov 17, 2018

image

image

this would be the menu after the latest changes I made. ( with centered menu )

Regarding the integration to the Chrome / Header: Can we make a new issue for that and implement it in a later version ? I think the mockups would be useful for that.

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

UniversalSuperBox commented Nov 19, 2018

Can you test desktop mode with this added? It seems like the switch in the browser settings does nothing after I install this PR. New tabs, new windows, rebooting... Everything still gets the mobile useragent.

@cibersheep

This comment has been minimized.

Copy link

cibersheep commented Nov 19, 2018

this would be the menu after the latest changes I made. ( with centered menu )
Looks nice. Good job!

Regarding the integration to the Chrome / Header: Can we make a new issue for that and implement it in a later version ? I think the mockups would be useful for that.
Consider that done

@ghost

This comment has been minimized.

Copy link

ghost commented Nov 19, 2018

Yes It seems like the switch in the browser settings to desktop mode it does nothing.

@balcy

This comment has been minimized.

Copy link
Collaborator

balcy commented Nov 20, 2018

Can you test desktop mode with this added? It seems like the switch in the browser settings does nothing after I install this PR. New tabs, new windows, rebooting... Everything still gets the mobile useragent.

strange, for me https://www.whatismybrowser.com/detect/what-is-my-user-agent behaves like that:
Change from mobile to desktop take effect after a page reload
Change from desktop to mobile take effect after browser restart
But why should this PR influence desktop mode?

@balcy balcy force-pushed the balcy:xenial_-_zoomcontrol branch from 1eab532 to 845d8a4 Nov 20, 2018

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

UniversalSuperBox commented Nov 22, 2018

Looks like the desktop mode setting isn't working correctly with or without this PR in some cases. This PR just aggravates the issue. Carry on.

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

UniversalSuperBox commented Nov 22, 2018

If we are unable to fix #109, I advise against merging this change. I don't understand why this makes it worse, but when I have it installed the first tab of every browser window I open is always "mobile" even if desktop mode is set.

@balcy

This comment has been minimized.

Copy link
Collaborator

balcy commented Nov 22, 2018

not sure if this is related, but I have found a binding loop in the logs: WebViewImpl.qml:703:5: QML UbuntuShape: Binding loop detected for property "height" which I've fixed in the latest commit

@balcy balcy force-pushed the balcy:xenial_-_zoomcontrol branch from 127b268 to 1a6974e Nov 25, 2018

@jezek

This comment has been minimized.

Copy link

jezek commented Nov 27, 2018

Hello, today I updated to rc channel on xenial and wanted to try out this pr. I followed @UniversalSuperBox instructions:

This PR can be installed on your device running Ubuntu Touch 16.04 from the devel or rc channel with the following command:

sudo ubports-qa install morph-browser 89

After that, restart morph-browser if it's running. There is now a "Page Menu" option in the overflow menu which you can use.

At first, the command failed with insufficient space error, but then I followed some instructions to free some space, and the command was successful, but I can't see the "Page Menu", even after phone reboot .

What am I doing wrong?

@balcy balcy force-pushed the balcy:xenial_-_zoomcontrol branch from 1a6974e to 528e2b5 Nov 27, 2018

@balcy

This comment has been minimized.

Copy link
Collaborator

balcy commented Nov 27, 2018

@jezek nothing wrong, the current system does require a rebase of the PR each time the xenial branch is updated... so should work after the build is finished

@jezek

This comment has been minimized.

Copy link

jezek commented Nov 27, 2018

@jezek nothing wrong, the current system does require a rebase of the PR each time the xenial branch is updated... so should work after the build is finished

@balcy how do I know, when the build is finished?

@balcy

This comment has been minimized.

Copy link
Collaborator

balcy commented Nov 27, 2018

if there is written "all checks passed" (and no longer pending checks) it is ok. For morph the build only takes a few minutes: https://ci.ubports.com/blue/organizations/jenkins/morph-browser/detail/PR-89/13/pipeline
so it is already ok now

@jezek

This comment has been minimized.

Copy link

jezek commented Nov 28, 2018

Ok, everything is all right now. This PR works as temporary workaround for #107 issue for me. Thank you.
Is there something, that needs to be tested?
The device is FP2.

@balcy balcy referenced this pull request Dec 1, 2018

Closed

Page menu doesn't show #117

@UniversalSuperBox
Copy link
Member

UniversalSuperBox left a comment

Pending final review by @cibersheep this is ready for merge.

@UniversalSuperBox
Copy link
Member

UniversalSuperBox left a comment

Scratch that. On further testing, it seems like the zoom factor is not set on the first load of a host.

I set my zoom factor to 25%, opened a new tab, then opened a webpage for the first time. It appeared at 100% zoom.

browser at 100% zoom

When opening the zoom menu, 25% is shown as the current zoom level. If I tap on + Zoom, it goes up to 110% and the page displays at 110%. If I reset the zoom, it goes to 25% and the page displays at 25%.

If I load the host again by doing any of the following, the zoom level is set correctly:

  • Close and reopen the browser
  • Open the host in a new tab

If I reload the already-opened page which has incorrect zoom, the zoom factor does not change.

@balcy

This comment has been minimized.

Copy link
Collaborator

balcy commented Dec 18, 2018

yes I can reproduce that and will look into it on thursday.

balcy added some commits Nov 4, 2018

default zoom factor for webbrowser is configurable in settings
add new page menu for:
- setting the current zoom factor for the view
- save current page as '.mhtml'
- save current page as PDF
- view source of current page
apply new default zoom factor to the current page (if it does not hav…
…e page specific zoom set)

add reset icon for default zoom setting (sets it back to 100%)
- move "view source" to main menu
- add "save as HTML / PDF" to main menu
- new dialog for saving current page as HTML or PDF
- add "Zoom" to main menu (instead of "page menu")
- "page menu" was renamed "zoom menu" and is always positioned centered
- zoom menu is closed if context menu or link menu is opened
- zoom menu gets new items "reset" (set zoom to default) and "save" (set current zoom as default)
- context menu is closed if zoom menu or link menu is opened
- remember the current zoomFactor in the zoomController
- set it as zoomFactor of the webview when loading is finished
- zoom menu items only available while the page is not loading, current zoom text not shown while page is loading

@balcy balcy force-pushed the balcy:xenial_-_zoomcontrol branch from cc8e0da to e69ff66 Dec 20, 2018

@balcy balcy requested a review from UniversalSuperBox Dec 20, 2018

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

UniversalSuperBox commented Dec 21, 2018

It looks like a custom zoom factor is not saved per-site. Instead, the zoom for every page is reset every time the browser is reopened. Is that expected?

@balcy

This comment has been minimized.

Copy link
Collaborator

balcy commented Dec 21, 2018

Currently yes, because we don't have a database for site/domain specific zoom factors yet. (just one global setting for the default zoom factor)
The user can change the zoom for the current tab with the keyboard shortcuts or zoom in / out buttons, and this factor is remembered until the tab or browser closes.

@UniversalSuperBox UniversalSuperBox merged commit d017521 into ubports:xenial Dec 21, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@balcy balcy deleted the balcy:xenial_-_zoomcontrol branch Dec 21, 2018

@leftside2

This comment has been minimized.

Copy link

leftside2 commented Jan 8, 2019

Error: You don't have enough free space in /var/cache/apt/archives/. after executing 'sudo ubports-qa install morph-browser 89'

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

UniversalSuperBox commented Jan 8, 2019

This PR has been merged and will be released with OTA-7 tomorrow. No need to install it manually! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment