Touch support for Gwenview
Needs ReviewPublic

Authored by steffenh on Thu, Jul 5, 11:06 AM.

Details

Reviewers
ngraham
Group Reviewers
Gwenview
Summary

Add touch support for Gwenview. At moment one finger scrolling, pinch gesture for zoom and rotate and one finger
swipe gesture to go to next or previous image is working with some little issue.

It is my first time I am modify the source code, so if I make same mistake please let me now.

CCBUG: 378021

Test Plan

Browse mode

  • test, if mouse and touchpad working as previously: scrolling, selection and drag and drop

all follow tests with touchscreen:

  • on finger scrolling, test if uwanted drag events or selection events occur
  • two finger scrolling, test if uwanted drag events or selection events occur
  • test drag and drop action (need a TabAndHold gesture before moving the touch point)
  • pinch gesture for zoom, check sensitive and unwanted behavior
  • check double touch, if system setting is set to: open file or directory with a double click

View Mode

  • test, if mouse and touchpad working as previously: scrolling, selection and drag and drop

all follow tests with touchscreen:

  • one finger panning, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • two finger panning, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • one finger swipe gesture, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • two finger swipe gesture, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • pinch gesture for zoom and rotate, check sensitive and unwanted behavior
  • drag and drop action (need a TabAndHold gesture before moving the touch point)
  • test double touch to toggle between view mode and fullscreen mode.

Thumbnail bar in view Mode:

  • test, if mouse and touchpad working as previously: scrolling, selection and drag and drop

all follow tests with touchscreen:

  • one finger scrolling, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • two finger scrolling, test if uwanted drag events or selection events occur
  • drag and drop action (need a TabAndHold gesture before moving the touch point)

Fullscreen Mode

  • test, if mouse and touchpad working as previously: scrolling, selection and drag and drop

all follow tests with touchscreen:

  • one finger panning, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • two finger panning, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • one finger swipe gesture, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • two finger swipe gesture, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • pinch gesture for zoom and rotate, check sensitive and unwanted behavior
  • drag and drop action (need a TabAndHold gesture before moving the touch point)
  • check if you get access to the fullscreen thumbnail bar on the top screen border

Thumbnail bar in fullscreen Mode:

  • test, if mouse and touchpad working as previously: scrolling, selection and drag and drop

all follow tests with touchscreen:

  • one finger scrolling, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • two finger scrolling, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • drag and drop action (need a TabAndHold gesture before moving the touch point)

Diff Detail

Repository
R260 Gwenview
Branch
arcpatch-D13901
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1050
Build 1063: arc lint + arc unit
steffenh requested review of this revision.Thu, Jul 5, 11:06 AM
steffenh created this revision.
rkflx added a subscriber: rkflx.Thu, Jul 5, 10:07 PM

Thanks for working on touch support in general and your patch in particular! I think this feature makes for a nice addition to Gwenview.

It is my first time I am modify the source code, so if I make same mistake please let me now.

That's no problem at all, we are happy to help you polish the code.

However, before that happens I'd like to verify that the feature is working properly (if that's okay with you). I don't have access to touch-enabled hardware currently, so please bear with me for a couple of days (I hope a regular touchpad will be enough to test this, or will I be needing a touchscreen?).


BTW, is you patch solving https://bugs.kde.org/show_bug.cgi?id=378021? If so, you can add

BUG: 378021

to your summary, so the bug will get closed automatically once the patch lands.

Also, please make sure that your patch does not contain unrelated changes. For example in documentviewcontainer.h and elsewhere you are unnecessarily removing newlines.

working with some little issue

Could you detail what those issues are?

ngraham requested changes to this revision.Fri, Jul 6, 1:39 AM
ngraham added a subscriber: ngraham.

Thanks for the patch! I've got touch hardware and am excited to test this out. I should be able to do that within the next day. In the meantime, I noticed some style issues with your code, which I've pointed out below. Here's some light reading on the subject: https://techbase.kde.org/Policies/Frameworks_Coding_Style

app/folderviewcontextmanageritem.cpp
31

No reason to change the whitespace here, methinks.

212

Unnecessary comment

app/startmainpage.cpp
36

Unnecessary whitespace change

lib/documentview/documentview.cpp
44

Unnecessary whitespace change

48

Unnecessary whitespace change

104

Unnecessary whitespace change

454

If the reason why this code is commented out is because the features haven't been implemented yet, please add a comment indicating this.

692

Coding style: opening braces in if statements go on the same line (do the same below, too).

695

Coding style: add spaces around the = (do the same below, too).

705

Coding style: this line is very long and a bit difficult to read. Might be better formatted with variables, like so:

const QPointF start = touchevent->touchPoints().at(0).startPos();
const QPointF end = touchevent->touchPoints().at(0).pos();
QPoint diff = start - end;
711

Coding style: comments should be in English. @rkflx (or many others) can probably provide an English translation for you.

712

Coding style: space after the <

714

Coding style: indent this line (do the same below, too).

726

Coding style: unnecessary whitespace

738

Coding style: insufficient indentation for this level and everything below it

753

coding style: add whitespace around the ,

lib/documentview/documentviewcontainer.cpp
55 ↗(On Diff #37181)

Unnecessary whitespace change

lib/documentview/documentviewcontainer.h
34 ↗(On Diff #37181)

Unnecessary whitespace change

39 ↗(On Diff #37181)

Unnecessary whitespace change

91 ↗(On Diff #37181)

Unnecessary whitespace change

lib/thumbnailview/thumbnailview.cpp
39

Unnecessary whitespace change

This revision now requires changes to proceed.Fri, Jul 6, 1:39 AM
steffenh marked 2 inline comments as done.Fri, Jul 6, 11:07 AM

Hi @rkflx,
thanks for your testing.
I think you need a touchscreen to test this, on my Laptop the touchpad don't work with this, but I think the touchpad is broken.

The code had a problem with draganddrop, sometimes an unwanted drag will start on touch. I have found this can crash gwenview if it happen in the wrong place.
Sometimes the panning in the dokumentview don't work, you need to touch on the toolbar, after this the pannig is working, why I don't know at the moment.

Hi @ngraham,
thanks for your time to look after my code.
I will change my style of the code and post a new diff (after I have worked out how it works).

Sounds good. You don't need to post a new diff; just make your changes and update the current one with arc diff (Thanks for using arc, by the way!)

steffenh updated this revision to Diff 37250.Fri, Jul 6, 12:16 PM

implement changes based on review comments

steffenh edited the summary of this revision. (Show Details)Fri, Jul 6, 12:18 PM
steffenh updated this revision to Diff 37272.Fri, Jul 6, 3:12 PM

Fix some crashes on DragandDrop in dokumentview

to start draganddrop in the dokumentview you need first tab and hold gesture for 400 milliseconds and then moving the touch
point. I don't know is this sensible or not, but I need a difference in the touch for panning, swipe and draganddrop.

rkflx added a comment.Fri, Jul 6, 11:08 PM

Thanks for the updates. As I mentioned before, I'll review the code later in detail (once most things are confirmed to work as they should).

Here's what I found when trying to test with a "precision touchpad": In libinput debug-gui I can see swipe (up to four fingers) and pinch gestures from the touchpad, and in Evince and GNOME's image viewer multitouch is working beautifully in a Wayland session. However, I cannot get it to work in both Gwenview and Qt's gesture example app

Unless there is some trick I missed, I guess that's a problem elsewhere in the stack, so for now we have to find testers with a real touchscreen. Once @ngraham tested your patch in-depth and we got everything working, I'd like to get at least a second opinion from someone with a real touchscreen for the final version of the patch (maybe then we can ask Oliver).

to start draganddrop in the dokumentview you need first tab and hold gesture for 400 milliseconds and then moving the touch
point. I don't know is this sensible or not, but I need a difference in the touch for panning, swipe and draganddrop.

As long as that does not regress drag handling for mouse users (which it did not in a quick check), I think that conceptual approach is fine. You could also try to find other applications which support dragging and swiping on a touchscreen, and look into how they solved it.

As for panning, I would suggest not to enable hold-to-initiate-drag there (like you are currently doing, if I skimmed the code correctly?). That's because you can then keep your finger on the screen when resuming panning later on. IOW, dragging should only work when not zoomed in (identical to how it works with a mouse), so perhaps you should check for !canPan()?

rkflx added a comment.Fri, Jul 6, 11:19 PM

Two more things:

  • Please also test swiping in View mode when one of the items is a video. In case that's not working yet, I'm not saying that should be solved in this patch (would be great, of course), but at least a bug should be filed so we don't forget about it.
  • In Browse mode, does a pinch gesture change the zoom level?

You could also add a "Test Plan" to your summary, so Nate will know exactly which parts of Gwenview to test and what features to look for.

libinput debug-gui shows that my touchpad and touchscreen can both can see two, three, and four finger swipes, two-finger pinches, two-finger rotation, and two finger scrolling.

Results of testing:

Browse mode

  • Two-finger scroll with a touchpad: no change, works fine
  • One-finger touch-and-drag with a touchscreen: draws a selection marquee and also scrolls. If you touch-and-dragged over a picture, it scrolls and also drags the image, resulting in a drag-and-drop menu when you remove your finger. Produces in big ugly white areas if you scroll beyond the content area.
  • Pinch in and out on a touchscreen: interpreted as a touch-and-drag instead of a pinch-zoom as it should

View Mode

  • Two-finger scroll over the thumbnail strip with a touchpad: no change, works fine
  • One-finger touch-and-drag on the thumbnail strip with a touchscreen: scrolls, but the thumbnail that was under your finger gets dragged and again you see the drag-and-drop menu when you release your finger. Also produces big ugly white areas if you scroll beyond the content area.
  • Three or four-finger swipe over the image on a touchpad: doesn't work; not implemented?
  • One-finger swipe left and right on a touchscreen: works, but there's no horizontal transition animation, so it looks really weird. Would be nice to have an animated effect of the image following your finger and moving to the left or right.
  • Two-finger pinch in and out on a touchpad: doesn't work; not implemented? This is the most critical thing to get working for https://bugs.kde.org/show_bug.cgi?id=378021.
  • Two-finger pinch in and out on a touchscreen: More or less works, it's much too sensitive; zoom should be slower and more gradual. Also, performance is pretty bad, and the zoom is quite choppy.
  • Two-finger pan on a zoomed-in image on a touchpad: no change, works fine
  • One-finger touch-and-drag to pan a zoomed-in image: doesn't work, and also horizontal drags are interpreted as swipe gestures and inappropriately move to another image
  • Two-finger rotate with a touchpad: doesn't work, not implemented?
  • Two-finger rotate with a touchscreen: works great, but is inappropriately triggered with you put two fingers down and move both of them in an arc (it should only trigger when one finger is held still and the other one moves in an arc)

It's getting late here and my brain is turning off, but I can test mode use cases as people come up with them,

rkflx added a comment.EditedSat, Jul 7, 6:59 AM

libinput debug-gui shows that my touchpad and touchscreen can both can see two, three, and four finger swipes, two-finger pinches, two-finger rotation, and two finger scrolling.

(Can you really see two-finger swipes, in contrast to two-finger scrolling?)

Results of testing:

Thanks for testing! I think such detailed feedback is really helpful. +1 for most of it, some additional commentary below:

View Mode

  • One-finger swipe left and right on a touchscreen: works, but there's no horizontal transition animation, so it looks really weird. Would be nice to have an animated effect of the image following your finger and moving to the left or right.

Agreed, but I would suggest to work on this in a separate patch (or at least file a bug).

@ngraham Is this working in Okular for you? If not, I would suggest to focus on touchscreen gestures here, and change the line in the summary to a CCBUG only.

  • Two-finger pinch in and out on a touchscreen: More or less works, it's much too sensitive; zoom should be slower and more gradual. Also, performance is pretty bad, and the zoom is quite choppy.

Perhaps this first needs work (in another patch) to make it work smoothly for zooming by scrolling and pressing Ctrl?

It's getting late here and my brain is turning off, but I can test mode use cases as people come up with them,

More ideas for what to touch on:

  • Start page
  • various tabs of the sidebar
  • thumbnail bar in fullscreen View mode
  • swiping in View mode when one of the items is a video

Hi @ngraham,

many thanks for your time to test this.

I will work on the points you have found, but I will concentrate on the touchscreen at the moment. If I get the touchscreen working, perhaps I can try the touchpad.

One-finger touch-and-drag with a touchscreen: draws a selection marquee and also scrolls. If you touch-and-dragged over a picture, it scrolls and also drags the image, resulting in a drag-and-drop menu when you remove your finger. Produces in big ugly white areas if you scroll beyond the content area.

Yes, this is a problem, I am working at the moment. The reason for this is, Qt synthesized a mouse click from the touch, I need to catch this and let only the right mouse clicks through.

  • Pinch in and out on a touchscreen: interpreted as a touch-and-drag instead of a pinch-zoom as it should

The pinch on this, is not implemented at this moment, I was not aware of this feature for Gwenview. I will implement it later.

  • One-finger touch-and-drag on the thumbnail strip with a touchscreen: scrolls, but the thumbnail that was under your finger gets dragged and again you see the drag-and-drop menu when you release your finger. Also produces big ugly white areas if you scroll beyond the content area.

Is the same as above. A synthesized mouse click from Qt start an unwanted drag. The big white area is a "feature" from the Qt QScroller.

  • Three or four-finger swipe over the image on a touchpad: doesn't work; not implemented?

I am using the Qt TouchEvent and GestureEvent for this, I have the feeling the touch on the touchpad will not land in the Event. Bug or feature from Qt??

  • One-finger swipe left and right on a touchscreen: works, but there's no horizontal transition animation, so it looks really weird. Would be nice to have an animated effect of the image following your finger and moving to the left or right.

For the image change I use the "build in" function from Gwenview, also the same effect if you press the next button in the toolbar.

  • Two-finger pinch in and out on a touchscreen: More or less works, it's much too sensitive; zoom should be slower and more gradual. Also, performance is pretty bad, and the zoom is quite choppy.

I can reduce the sensitive for this. At the moment I have a problem to set the center of the zoom to the center of the pinch gesture, I have not found the right mapping, so this result in a jumpy zoom effect.

  • One-finger touch-and-drag to pan a zoomed-in image: doesn't work, and also horizontal drags are interpreted as swipe gestures and inappropriately move to another image

Yes, sometimes this works, try to touch the toolbar button "next image", or touch on the image counter in the bottom of the window, after this is working for me. I don't know the reason for this, but I am looking for a solution. Perhaps I need to write a own event handler for this.

  • Two-finger rotate with a touchscreen: works great, but is inappropriately triggered with you put two fingers down and move both of them in an arc (it should only trigger when one finger is held still and the other one moves in an arc)

I will change the trigger for the rotation, so it only triggers if the center is stationary.

steffenh updated this revision to Diff 37467.Mon, Jul 9, 4:32 PM

Fix some issure found in the last testing from @ngraham

  • no unwanted drag action in browse mode
  • to start draganddrop you need a TabAndHold Gesture before you move the touchpoint
  • set QScroller OvershootPolicy to always off, no big ugly white areas anymore
  • one, two or more finger swipe in View mode to change image
  • reduce sensitify for zoom
  • one finger paning for image in view mode
  • draganddrop in view mode, after TabAndHold gesture
  • make pinch gesture for rotation more stationary to suppress unwanted rotations

add Test Plan to summary

TODO

  • QScroller for all scrollbar sidebars
  • new Drag Action (TabAndHold) for drag actions in all sidebars
  • Zooming in Browse mode
  • make zooming behavior in View mode nicer
  • need we multiple selection with the touchscreen in Browse mode?
  • many variable to modify the sensitivity, are a result of testing on my device, I don't know if on other devices the

sensitivity equal or not. Have we system variable for the touchscreen behavior?

  • if the system mouse behavior is set to double-click to open a file and directory, tap on touchscreen don't open Image

(double tap don't work)

  • two finger panning in View mode

PS:
do you want I work through my TODO list before I post the next diff, or is step per step ok.

steffenh edited the summary of this revision. (Show Details)Mon, Jul 9, 4:33 PM
steffenh edited the test plan for this revision. (Show Details)
rkflx added a comment.Mon, Jul 9, 7:47 PM

Thanks for the updates, glad to see your fast progress.

  • need we multiple selection with the touchscreen in Browse mode?

For now I'd say we should defer that particular feature to a future patch. This probably needs design work/ideas/research on how that interaction should work, not only in Gwenview, but in all of KDE's apps.

For example on some platforms there is a special selection mode which can be entered via a button or through tap-and-hold, after which a single tap allows to toggle the selection for the rest of the images. In other words, this is more than a simple change because it requires changes to the UI.

  • many variable to modify the sensitivity, are a result of testing on my device, I don't know if on other devices the sensitivity equal or not. Have we system variable for the touchscreen behavior?

Not sure how much difference there is between your and Nate's touchscreen behaviour, but generally speaking Gwenview would not be the right place for such configuration switches (if they are needed at all).

do you want I work through my TODO list before I post the next diff, or is step per step ok.

It's probably too much to ask for testing everything after every update, but if you want to get feedback on a particular change or addition, I'd say just update the Diff. In any case detailed testing of the final result will be needed.

Excellent progress. You managed to resolve nearly all the problems I encountered with the first version of this patch! Behavior-wise, here's all I found with this latest version:

  • Two-finger pinch zooming in View Mode is better, but still too sensitive IMHO. We don't yet have a global KCM to control touchscreen behavior, but it's inevitable that we'll get one. In the meantime, let's shoot for a sane default.
  • Two-finger pinch zooming in View Mode still doesn't center the zoom on the midpoint between your fingers, but I think this is a known issue you already mentioned?
  • It's still possible to one-finger swipe to the next and previous image when zoomed in, which results in a lot of unintended swipes when trying to pan around a zoomed-in image with one finger. I would advocate that you should only be able to (for example) one-finger swipe to the left if the zoomed-in viewport is already touching the left-hand side of the image.

Hi @rkflx,

For now I'd say we should defer that particular feature to a future patch. This probably needs design work/ideas/research on how that interaction should work, not only in Gwenview, but in all of KDE's apps.

Ok, I will ignore this at the moment.

It's probably too much to ask for testing everything after every update, but if you want to get feedback on a particular change or addition, I'd say just update the Diff. In any case detailed testing of the final result will be needed.

Ok, I will work trough my TODO list.

Hi @ngraham,
thank you for testing

  • Two-finger pinch zooming in View Mode is better, but still too sensitive IMHO. We don't yet have a global KCM to control touchscreen behavior, but it's inevitable that we'll get one. In the meantime, let's shoot for a sane default.

I will reduce the sensitive, no problem.

  • Two-finger pinch zooming in View Mode still doesn't center the zoom on the midpoint between your fingers, but I think this is a known issue you already mentioned?

Yes, this is a problem I am working at the moment. I think I have found a solution, but I will test it first on my device.

  • It's still possible to one-finger swipe to the next and previous image when zoomed in, which results in a lot of unintended swipes when trying to pan around a zoomed-in image with one finger. I would advocate that you should only be able to (for example) one-finger swipe to the left if the zoomed-in viewport is already touching the left-hand side of the image.

Ok, this is a good idea, I will implement this.

I hope for the weekend, I will have a new version of the patch.

steffenh updated this revision to Diff 37730.Sat, Jul 14, 9:18 AM
steffenh edited the test plan for this revision. (Show Details)

Fix some issure, implented some things

new features:

  • pinch gesture for zooming in browse mode
  • double touch for toggle fullscreen in view mode
  • reimplented two fingers scrolling / panning in browse and view mode
  • double touch for the open image / directory in browse mode, if the system mouse behavior set to "double click to open

file.."

  • add one finger scrolling to various sidebars

fixed issure:

  • reduce sensivity for zooming (insert a delay for same millisecond, to suppress same unwanted zooming)
  • swipe gesture to change image, only works now if viewport of the image at the left or right border
steffenh edited the test plan for this revision. (Show Details)Sat, Jul 14, 9:19 AM

The progress here is excellent! Here are my comments with the latest revision of the patch:

  • Pinch zooming in View mode is just a small amount too slow now. :)
  • Pinch zooming in View mode seems to try to center the zoom on the midpoint between your fingers, but it's not as accurate as it needs to be. It works well if you pinch zoom while also moving both fingers--in this case it does a zoom-and-pan in the right general area. But if you just do a pinch zoom without moving the fingers too, the zoom seems centered somewhere else.
  • Pinch zooming in View mode is rather laggy and could be smoother to provide a really optimal user experience.
  • Pinch zooming in Browse mode (nice feature!) is quite a bit too slow.
  • While in View mode, when I touch a thumbnail in the thumbnail bar, mpw it sometimes takes two touches to activate. This seems to happen a lot when I change direction (e.g. when I touch a picture to the left of the current one four times, and then touch a picture to the right of the current one, very often that touch isn't registered properly)
  • The Folders tab of the sidebar should activate items on a single-touch even if double-click is using used (clicks != touches)

Hi, @ngraham,

thank you again for your time, to test this patch.

  • Pinch zooming in View mode is just a small amount too slow now. :)
  • Pinch zooming in View mode is rather laggy and could be smoother to provide a really optimal user experience.
  • Pinch zooming in Browse mode (nice feature!) is quite a bit too slow.

I will increase the sensitivity for the zoom, maybe a value between this version and the previous version?

  • Pinch zooming in View mode seems to try to center the zoom on the midpoint between your fingers, but it's not as accurate as it needs to be. It works well if you pinch zoom while also moving both fingers--in this case it does a zoom-and-pan in the right general area. But if you just do a pinch zoom without moving the fingers too, the zoom seems centered somewhere else.

I think I don't understand this right, what you mean with "do a pinch zoom without moving the fingers".

  • While in View mode, when I touch a thumbnail in the thumbnail bar, mpw it sometimes takes two touches to activate. This seems to happen a lot when I change direction (e.g. when I touch a picture to the left of the current one four times, and then touch a picture to the right of the current one, very often that touch isn't registered properly)

Ok, I will give the tab gesture a little bit wiggel room, (I think the reason for this issure is, you have a little moving between touch begin and touch end).

  • The Folders tab of the sidebar should activate items on a single-touch even if double-click is using used (clicks != touches)

You mean we don't take the mouse behavior for touch behavior? Ok, I will change this.

Hi, @ngraham,

thank you again for your time, to test this patch.

You're very welcome! It's coming along nicely.

  • Pinch zooming in View mode is just a small amount too slow now. :)
  • Pinch zooming in View mode is rather laggy and could be smoother to provide a really optimal user experience.
  • Pinch zooming in Browse mode (nice feature!) is quite a bit too slow.

I will increase the sensitivity for the zoom, maybe a value between this version and the previous version?

Couldn't hurt to try it out, at least! :)

  • Pinch zooming in View mode seems to try to center the zoom on the midpoint between your fingers, but it's not as accurate as it needs to be. It works well if you pinch zoom while also moving both fingers--in this case it does a zoom-and-pan in the right general area. But if you just do a pinch zoom without moving the fingers too, the zoom seems centered somewhere else.

I think I don't understand this right, what you mean with "do a pinch zoom without moving the fingers".

I mean when you do a pinch zoom and a pan at the same time, basically, You zoom by pinching, while also moving the pinching fingers in the same direction to pan the view at the same time that the pinch zoom happens.

  • The Folders tab of the sidebar should activate items on a single-touch even if double-click is using used (clicks != touches)

You mean we don't take the mouse behavior for touch behavior? Ok, I will change this.

Right, the click setting only applies to clicks. With touch, double-click doesn't exist (and double-tap is a very challenging gesture); the whole UI should use the single-touch paradigm.

steffenh updated this revision to Diff 38176.Sat, Jul 21, 11:51 AM
steffenh edited the test plan for this revision. (Show Details)

Fix some issue

  • adjust zooming speed
  • give tab gesture some wobble room
  • ignore the system behavior for double-click, activate items, always with a single touch
  • fix issue with drag and drop in browse mode, now drag the right image
  • fix issue in view mode with multiple images, now double touch to toggle fullscreen works
  • fix issue with toolbar in fullscreen view mode, now fade in the toolbar if you touch the up border area of the screen
  • adjust zoom center to pinch center (I have a feeling the zoom function doesn't like me)