Opened 3 years ago

Closed 2 years ago

Last modified 17 months ago

#3455 closed defect (fixed)

Browse: show link palette

Reported by: manuq Owned by: humitos
Priority: High Milestone:
Component: Browse Version: Git as of bugdate
Severity: Major Keywords: 13.1.0, r+, olpc-test-pending
Cc: erikos, godiard, humitos, manuq Distribution/OS: OLPC
Bug Status: Assigned

Description

Replace the default WebKitGTK menu for our custom palette.

For links the options were: follow link, follow link in new tab, keep link, copy link, start download.

And for images were: keep image, copy image.

Attachments (6)

first_palettes.patch (13.4 KB) - added by erikos 3 years ago.
First patch towards palette support for link and images
0001-Show-Sugar-s-palette-when-right-click-is-pressed-SL-.patch (19.1 KB) - added by humitos 2 years ago.
0001-Show-Sugar-s-palette-when-right-click-is-pressed-SL-.2.patch (19.1 KB) - added by humitos 2 years ago.
v2 - "Keep link" implemented (and Simon added)
0001-Show-Sugar-s-palette-when-right-click-is-pressed-SL-.3.patch (19.6 KB) - added by humitos 2 years ago.
v3 - close the palette after clicking on the MenuItem
0001-Use-PaletteMenuItem-instead-local-SugarMenuItem-SL-3.patch (9.6 KB) - added by humitos 2 years ago.
0001-Use-PaletteMenuItem-instead-local-SugarMenuItem-SL-3.2.patch (9.7 KB) - added by humitos 2 years ago.
Last patch rebased

Download all attachments as: .zip

Change History (35)

comment:1 Changed 3 years ago by manuq

  • Component changed from untriaged to Browse
  • Keywords 12.1.0 added
  • Milestone changed from Unspecified by Release Team to 0.96
  • Owner set to manuq
  • Status changed from new to assigned
  • Version changed from Unspecified to Git as of bugdate

comment:2 Changed 3 years ago by manuq

  • Cc erikos added

comment:3 Changed 3 years ago by manuq

  • Priority changed from Unspecified by Maintainer to High

comment:4 Changed 3 years ago by manuq

A related bug is #1222, we need to wrap the images menu that WebKit provides when right-clicking on them.

comment:5 Changed 3 years ago by manuq

Another related old bug: #946 .

Changed 3 years ago by erikos

First patch towards palette support for link and images

comment:6 Changed 3 years ago by erikos

  • Bug Status changed from Unconfirmed to Assigned
  • Distribution/OS changed from Unspecified to OLPC
  • Severity changed from Unspecified to Major

The patch above does display the palettes for images and links. The link options "Following a link" and "Following it in a new tab" do work. The options for copying a link and an image to the clipboard do NOT work and downloading an image or a clipboard neither. The behavior is different in the sense that the palette will not popdown when the mouse moves away from the invoker widget (link or image), which has worked before. The palette needs to be dismissed by clicking on the underlying widget or it will go as well away when you move out of the palette itself.

  • The downloading part needs to be simply implemented

comment:7 Changed 3 years ago by manuq

Great advance! Some feedback:

  • The title of the link is missing at the top of the palette.
  • Some URL links do not display at the top of the palette, like the "Log in" link at the top of the SugarLabs wiki.
  • The order is wrong, the right order is in the description of this ticket.

comment:8 Changed 3 years ago by humitos

  • Keywords patch added

comment:9 Changed 2 years ago by humitos

  • Cc godiard humitos added

The problem is still visible on build 14

comment:10 follow-up: Changed 2 years ago by humitos

What should happen when the user click over the text itself? I mean, no link, no image, just text... and what should happen if that text is selected?

comment:11 Changed 2 years ago by humitos

I uploaded a new patch with some improvements to eriko's patch. Please, take a look at it and let me know if there are something that I can do to solve the issues that it has.

comment:12 Changed 2 years ago by humitos

I couldn't find what is the meaning of the Menu Item: "Keep link".

Changed 2 years ago by humitos

v2 - "Keep link" implemented (and Simon added)

comment:13 in reply to: ↑ 10 Changed 2 years ago by manuq

Replying to humitos:

What should happen when the user click over the text itself? I mean, no link, no image, just text... and what should happen if that text is selected?

This is common sense, you don't need to handle that.

comment:14 Changed 2 years ago by manuq

Great, v2 gets close to a solution. The problem I see is that the palette should be dismissed after you click in one item.

comment:15 Changed 2 years ago by humitos

  • Owner changed from manuq to humitos
  • Status changed from assigned to accepted

comment:16 Changed 2 years ago by manuq

  • Keywords 13.1.0 added; 12.1.0 removed
  • Milestone changed from 0.96 to 0.98

Changed 2 years ago by humitos

v3 - close the palette after clicking on the MenuItem

comment:17 follow-ups: Changed 2 years ago by manuq

  • Resolution set to fixed
  • Status changed from accepted to closed

Great, pushed as c3b6dd91 . Note that we still have unsolved the popup when the mouse is hover. This patch does the popup only for the right click. The popup when hover should be recovered in the future.

comment:18 in reply to: ↑ 17 Changed 2 years ago by humitos

Replying to manuq:

Great, pushed as c3b6dd91 . Note that we still have unsolved the popup when the mouse is hover. This patch does the popup only for the right click. The popup when hover should be recovered in the future.

This is followed here: #3967

comment:19 in reply to: ↑ 17 ; follow-up: Changed 2 years ago by erikos

Replying to manuq:

Great, pushed as c3b6dd91 . Note that we still have unsolved the popup when the mouse is hover. This patch does the popup only for the right click. The popup when hover should be recovered in the future.

This commit needs cleanup. There are debug logs in it, uncommented code, and it uses a local sugarmenuitem and not the one from the toolkit.

comment:20 in reply to: ↑ 19 Changed 2 years ago by manuq

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to erikos:

Replying to manuq:

Great, pushed as c3b6dd91 . Note that we still have unsolved the popup when the mouse is hover. This patch does the popup only for the right click. The popup when hover should be recovered in the future.

This commit needs cleanup. There are debug logs in it, uncommented code, and it uses a local sugarmenuitem and not the one from the toolkit.

Oh true. Reopening to fix it.

comment:21 Changed 2 years ago by erikos

  • Keywords p? added; patch removed

comment:22 Changed 2 years ago by humitos

  • Keywords r? added; p? removed

comment:23 Changed 2 years ago by manuq

Copy Image functionallity fixed by Gonzalo, dd7f9521 .

comment:24 Changed 2 years ago by erikos

  • Cc manuq added
  • Keywords r- added; r? removed

Humitos, can you rebase the above patch, it does not apply anymore. And please check if we have any layout regressions as we adjusted the PaleteMenuItem code recently.

Changed 2 years ago by humitos

Last patch rebased

comment:25 Changed 2 years ago by humitos

  • Keywords r? added; r- removed

erikos, I've just attached the new version of this patch that applies in master.

comment:26 Changed 2 years ago by erikos

  • Keywords r+ olpc-test-pending added; r? removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Pushed this one. To test please verify that the copy link, follow link etc Palette is working.

comment:27 Changed 2 years ago by greenfeld

I am getting slightly confused testing this one.

A lot of times when I try to keep a link, open a picture link it in a new tab, etc., Sugar as a whole crashes and gets restarted in OLPC 13.1.0 os20/Browse-148.

I have not tracked this down yet, and sometimes going back to a page/link which previously caused a crash does not cause it a second time, and sometimes going back a second time causes a crash when the first time to do something with a resource does not.

Seen on XO-1.75 & XO-4.

comment:28 Changed 2 years ago by dsd

Those crashes are probably gone in the latest sugar releases, included in build 21.

comment:29 Changed 17 months ago by dnarvaez

  • Milestone 0.98 deleted

Milestone 0.98 deleted

Note: See TracTickets for help on using tickets.