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.

Sign up for GitHub

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

Jump to bottom

(WIP) introduce autofill preview tooltip #10037

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

(WIP) introduce autofill preview tooltip #10037

wants to merge 2 commits into from
+132 −2

Conversation

bayramcicek
Copy link
Member

@bayramcicek bayramcicek commented Sep 12, 2024 •
edited
Loading

Change-Id: Ice62a3eb891a1653b5ec6ad5b10efe2fac0e914f

  • Resolves: #
  • Target version: master

Summary

  • we should show cell value previews in a popup while using autofill feature.
  • This will allow users to see what value will be filled in the current cell in advance.
  • use 'fixedtext' type for cell previews
  • add AutoFillPreviewTooltip class
  • requires: https://gerrit.libreoffice.org/c/core/+/172589

TODO

  • ...

Checklist

  • I have run make prettier-write and formatted the code.
  • All commits have Change-Id
  • I have run tests with make check
  • I have issued make run and manually verified that everything looks okay
  • Documentation (manuals or wiki) has been updated or is not required

@bayramcicek
Copy link
Member Author

bayramcicek commented Sep 12, 2024 •
edited
Loading

  • Using treeview: currently, popup is blocking the auto-fill (but the value inside the popup is true)
Peek.2024-09-12.11-19.mp4
  • Using fixedtext: looks good

Screenshot_20240912_120248

eszkadev
eszkadev reviewed Sep 13, 2024
View reviewed changes
browser/src/control/Control.AutoFillPreviewTooltip.ts Outdated
var currPos = {
// x: app.file.textCursor.rectangle.cX1,
x: app.file.viewedRectangle.cX1,
y: app.file.textCursor.rectangle.cY2,
Copy link
Contributor

Choose a reason for hiding this comment

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

textCursor -> where "caret" is while editing text inside cell
cellCursor -> position of selected cell

please look into docstate.js

Copy link
Member Author

Choose a reason for hiding this comment

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

I use:

var currPos = {
	x:
		app.calc.cellCursorRectangle.pWidth +
		app.calc.cellCursorRectangle.pX1 +
		5,
	y:
		app.calc.cellCursorRectangle.pHeight +
		app.calc.cellCursorRectangle.pY1 +
		5,
};

currently we have:

Peek.2024-09-16.12-30.mp4

issues

  • Popup width is too long
  • popup location should be updated per autofill selection

Copy link
Member Author

Choose a reason for hiding this comment

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

well, at least we can move the popup right now. I needed to close the popup before update it :)

Peek.2024-09-19.11-21.mp4

@bayramcicek bayramcicek force-pushed the private/bayram/autofill-preview-tooltip branch from bf26e73 to 6476ce4 Compare September 16, 2024 09:20
@bayramcicek bayramcicek force-pushed the private/bayram/autofill-preview-tooltip branch from 6476ce4 to 0169e9b Compare September 16, 2024 09:44
eszkadev
eszkadev reviewed Sep 17, 2024
View reviewed changes
browser/src/layer/tile/CanvasTileLayer.js Outdated

var topLeftPixels = this._twipsToCorePixels(topLeftTwips);
var offsetPixels = this._twipsToCorePixels(offset);
this._cellAutoFillAreaPixels = L.LOUtil.createRectangle(topLeftPixels.x, topLeftPixels.y, offsetPixels.x, offsetPixels.y);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we somehow put this into event's data? in the map.fire ( ... )
so we will not need to store it inside canvas layer class

@bayramcicek bayramcicek force-pushed the private/bayram/autofill-preview-tooltip branch from 0169e9b to ed42db0 Compare September 19, 2024 08:29
@bayramcicek bayramcicek changed the title (WIP): introduce autofill preview tooltip (WIP) introduce autofill preview tooltip Sep 19, 2024
bayramcicek and others added 2 commits September 19, 2024 14:15
@bayramcicek @gokaysatir
(WIP) introduce autofill preview tooltip
bc17307 
- we should show cell value previews in a popup while using autofill feature.
- This will allow users to see what value will be filled in the current cell in advance.
- use 'fixedtext' type for cell previews
- add AutoFillPreviewTooltip class
- requires: https://gerrit.libreoffice.org/c/core/+/172589

Signed-off-by: Bayram Çiçek <[email protected]>
Change-Id: Ice62a3eb891a1653b5ec6ad5b10efe2fac0e914f
@gokaysatir
Fix the position reading of auto fill preview tooltip.
e839a87 
Add a special case to JS dialog positioning next to Autofilter case.
Use a new object everytime as newPopupData.
Use the position read from the event.

Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: I6c3ad27914f109b6df42d780438f59149c3495e8
@gokaysatir gokaysatir force-pushed the private/bayram/autofill-preview-tooltip branch from ed42db0 to e839a87 Compare September 19, 2024 13:57
@gokaysatir
Copy link
Contributor

@bayramcicek hello, thanks for the PR :)

I sent a small update to fix some issues. But there are still things to look:

  • The position has a fixed offset. I hope it is easy to find.
  • I added one more special case to JSDialogs along with AutofillMarker case. @eszkadev may have a better idea for this type of popup windows.
  • Code may need some beautifications.

I hope this helps, thanks :)

eszkadev
eszkadev reviewed Sep 19, 2024
View reviewed changes
browser/src/control/Control.JSDialog.js
@@ -710,8 +710,10 @@ L.Control.JSDialog = L.Control.extend({
instance.updatePos();
}

if (instance.isAutofilter)
if (instance.isAutofilter && instance.id !== 'autoFillPreviewTooltip')
Copy link
Contributor

Choose a reason for hiding this comment

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

pleae do not use hardcoded ids

Copy link
Contributor

Choose a reason for hiding this comment

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

isn't that wrong due to centerDialogPosition above maybe?
or really we enter autofilter case?

Copy link
Contributor

Choose a reason for hiding this comment

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

We enter the autofilter case there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see isAutofilter is too general now: instance.isAutofilter = instance.isDocumentAreaPopup && this.map._docLayer.isCalc();

Can we check if instance.isAutoCompletePopup can be used here?

@bayramcicek
Copy link
Member Author

@bayramcicek hello, thanks for the PR :)

I sent a small update to fix some issues. But there are still things to look:

* The position has a fixed offset. I hope it is easy to find.

* I added one more special case to JSDialogs along with AutofillMarker case. @eszkadev may have a better idea for this type of popup windows.

* Code may need some beautifications.

I hope this helps, thanks :)

Thank you so much :) I will check them asap.

eszkadev
eszkadev reviewed Sep 23, 2024
View reviewed changes
browser/src/control/Control.JSDialog.js
this.calculateAutoFilterPosition(instance);
else if (instance.id === 'autoFillPreviewTooltip')
Copy link
Contributor

Choose a reason for hiding this comment

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

can be instance.isAutoCompletePopup?

eszkadev
eszkadev reviewed Sep 23, 2024
View reviewed changes
browser/src/control/Control.AutoFillPreviewTooltip.ts
clickToClose: '_POPOVER_',
id: 'autoFillPreviewTooltip',
canHaveFocus: false,
noOverlay: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

isAutoCompletePopup property might be needed

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

@eszkadev eszkadev eszkadev left review comments

@gokaysatir gokaysatir Awaiting requested review from gokaysatir

@caolanm caolanm Awaiting requested review from caolanm

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
CODE "red"
Projects
Collabora Online
Status: To Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bayramcicek @gokaysatir @eszkadev