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

Fixing cross platform app width #174

Merged
merged 10 commits into from
Jan 2, 2013

Conversation

PragTob
Copy link
Member

@PragTob PragTob commented Dec 20, 2012

Hopefully fixes #173

This contains quite some refactoring that's not essential.
What was essential was adding the event listeners after the shell.
Hopefully that works on all platforms. Everyone please check.

It also contains some refactorings that I thought were appropriate, open for comments and suggestions as always.

Most importantly, @ashbb @wasnotrice and @davorb please try it out on Mac and Windows to make sure that it really works on all platforms.

And travis - pleeaaassseee pass.

@PragTob
Copy link
Member Author

PragTob commented Dec 20, 2012

Ok travis is on board - how about everyone else? :-D

@ashbb
Copy link
Member

ashbb commented Dec 21, 2012

@PragTob

Thanks for the work. I tried to run sample2.rb with your commit on my Windows 7 and got this snapshot.

shoes4-sandbox-034.png

Umm,... I think the expected snapshot is the following.

shoes4-sandbox-035.png

@PragTob
Copy link
Member Author

PragTob commented Dec 21, 2012

Ahhhh.... thank you so much @ashbb - yep deinitely my fault but all tests green :-/ Will take a look at the causes of this and hopefully come up with a solution :-D Well maybe tomorrow or feel free to chime in if you know suspect why =)

Note to self: run more samples

@PragTob
Copy link
Member Author

PragTob commented Dec 22, 2012

Hi everyone,

a few frustrating hours later I think I know what the problem is, but can't really fix it.

It's the scroll bar (if I'm right) and it should probably be labeled a SWT bug... but maybe I'm just tired.

Check out this script:

require 'swt'

WIDTH = 500
HEIGHT = 400

# comment out for expected behaviour (and comment in the other shell definition)
@shell = ::Swt::Widgets::Shell.new(::Swt.display, ::Swt::SWT::V_SCROLL)
vb = @shell.getVerticalBar
puts 'vb size x: ' + vb.getSize.x.to_s

#@shell = ::Swt::Widgets::Shell.new(::Swt.display)
puts "what clienat area width should be: " + WIDTH.to_s
@shell.setBounds(@shell.computeTrim(100, 100, WIDTH, HEIGHT))
puts "What client area width actually is " + @shell.getClientArea.width.to_s

You can check out computeTrim - it basically should compute the bounds of the shell so that the clientArea (eg. how much space we really have) behaves as specified.

I get the following output though:

vb size x: 16
what clienat area width should be: 500
What client area width actually is 503

If I use a shell without styles (or even with all styles I tried but V_SCROLL) I get the following (expected) output:

what clienat area width should be: 500
What client area width actually is 500

I tried removing the vertical scrollbar in Shoes4 and everything worked fine just as expected.... however removing it can't really be the solution. Another problem, to, me is that the vertical scrollbar itself is 16 pixels wide - but the error is 3 pixels... so ummm. Dunno.

@wasnotrice
Copy link
Member

@PragTob thanks for looking into this. It really helps to have a better sense of what the problem is :)

@ashbb
Copy link
Member

ashbb commented Dec 25, 2012

@PragTob Sorry for my late reply.

I just checked out the above script (i.e. with V_SCROLL) and got the following output on my Windows 7.

vb size x: 17
what clienat area width should be: 500
What client area width actually is 500

Umm,... SWT seems to return the different size on each platform...

ashbb added a commit that referenced this pull request Dec 25, 2012
@PragTob
Copy link
Member Author

PragTob commented Dec 25, 2012

Well there seems to be a whole lot of this :-)

I've cooked up an idea for a fix in my mind by the way.. will probably take a stab at it this week after christmas - let's see :-)

@davorb
Copy link
Member

davorb commented Dec 26, 2012

Here's the output on OS X Mountain Lion.

vb size x: 0
what clienat area width should be: 500
What client area width actually is 500

Davor Babic
davor@davor.se

ashbb wrote:

require 'swt'

WIDTH = 500
HEIGHT = 400

comment out for expected behaviour (and comment in the other shell definition)

@shell = ::Swt::Widgets::Shell.new(::Swt.display, ::Swt::SWT::V_SCROLL)
vb = @shell.getVerticalBar
puts 'vb size x:' + vb.getSize.x.to_s

#@shell = ::Swt::Widgets::Shell.new(::Swt.display)
puts "what clienat area width should be:" + WIDTH.to_s
@shell.setBounds(@shell.computeTrim(100, 100, WIDTH, HEIGHT))
puts "What client area width actually is" + @shell.getClientArea.width.to_s

The default_options looked more like a constant to me, this way we
can also easily use them in the specs - which I like. This is
very debatable though so open for feedback!
This contains quite some refactoring that's not essential.
What was essential was adding the event listeners after the shell.
Hopefully that works on all platforms. Everyone please check.
This is the best solution I can think of right now and I really hope it works
for all platforms. After packing the shell with everything the error regarding
the desired app size is computed and is then continuously taken into
consideration.
About the width problems we discovered, at least on my Linux machine, I'll
probably go rewrite the example in Java and submit a bug report.
Oh and I removed my various puts statements :-)
@PragTob
Copy link
Member Author

PragTob commented Dec 30, 2012

Hi everyone,

This is the best solution I can think of right now and I really hope it works
for all platforms. After packing the shell with everything the error regarding
the desired app size (width) is computed and is then continuously taken into
consideration.
About the width problems we discovered, at least on my Linux machine, I'll
probably go rewrite the example in Java and submit a bug report.
Oh and I removed my various puts statements :-)

So, please try this out on Windows7 and OSX to see if the specs really work for everyone.
If you pulled this branch before, be careful/use a new one. I rebased against the current head in order to adjust some of the commits.

@PragTob
Copy link
Member Author

PragTob commented Dec 30, 2012

Oh by the way, if my measurements with gimp are any indication the real width is still 3 pixels or even 4 too much - but as I consider this an SWT bug (that I didn't find a better way to work around) and it would be confusing to people I decided to report the slightly wrong but expected width.

PragTob added a commit that referenced this pull request Jan 2, 2013
@PragTob PragTob merged commit 4c4f8b8 into shoes:master Jan 2, 2013
@PragTob
Copy link
Member Author

PragTob commented Jan 2, 2013

Specs passed on OSX and since it was OSX vs. Windows & Linux for now I assume it's working on Windows now. Would be cool if @ashbb or somebody else could check it though. If they don't please reopen #173

@ashbb
Copy link
Member

ashbb commented Jan 3, 2013

@PragTob Sorry for my late reply. Specs passed on Windows 7. But I got the below snapshot. The sample2.rb didn't work as I expected...

shoes4-sandbox-037.png

So, I reopen #173.

@PragTob
Copy link
Member Author

PragTob commented Jan 3, 2013

What's wrong with it? You mean that there is a scroll bar on the right?

That's weird indeed, get the same on Linux but why the hell.. and I thought this chapter was finally closed :-/

@PragTob
Copy link
Member Author

PragTob commented Jan 3, 2013

Oh and thanks for checking this out @ashbb :-)

So I looked into this and I did go back to 3e8e3c1 (a commit by me from the 17th December, way before the changes of this pull request) - and I can still scroll sample2.rb simply by getting the mouse in the app and using the scroll wheel I can scroll down.
The only difference seems to be that the scroll bar isn't shown - so I'd rather consider this a feature, the fault presumably is somewhere else.

edit: just found a commit where it works (no scrolling possible) - will maybe do a git bisect...

@PragTob
Copy link
Member Author

PragTob commented Jan 3, 2013

Ok so according to git bisect* this fault seems to have been introduced in 26f4a25 - if you reset to this commit and run sample2.rb you don't see a scroll bar but you can still scroll with the mouse wheel when you hover the app area.

But wait, maybe our apps have been "too big" all along and we just started noticing it when the scroll behaviour has been implemented because there literally wasn't any other way for us to notice that they are too big?

I think I probably should open a new issue for this?

* git bisect works the following way, you give it two commit hashes, one where you know that the problem doesn't exist and one where you know the problem does exist. It then performs a binary search always checking out commits and asking you if the problem is there in the particular commit that is checked out right no. I just ran sample2.rb and checked if I could scroll there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cross platform SWT issues with app width
4 participants