Re: Review Request 128369: Handle multiple gzip streams in a single file

2016-07-05 Thread David Faure 


> On July 4, 2016, 9:39 p.m., David Faure wrote:
> > src/kgzipfilter.cpp, line 331
> > 
> >
> > This is where you could add something like
> >  if (avail_in < 10) {
> >  return Ok; // come back when we have more
> >  }
> >  if (readHeader()) { ... }
> > no?
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> Then we'd need to somehow save what we currently have in the buffer and 
> concatenate with the next. I tested this with setting BUFFER_SIZE to 29 bytes 
> (the stream in Sune's test file is 28 bytes).
> 
> I don't really think handling an uncompressed stream after the compressed 
> one is something we really need to care about, so just ignoring the header I 
> think is good enough for now, or?

Ah right I thought we would get these bytes at the next iteration, but indeed 
that's now how next_in works.
OK.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128369/#review97103
---


On July 4, 2016, 10:33 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128369/
> ---
> 
> (Updated July 4, 2016, 10:33 p.m.)
> 
> 
> Review request for KDE Frameworks and Sune Vuorela.
> 
> 
> Repository: karchive
> 
> 
> Description
> ---
> 
> Handle multiple gzip streams
> 
> If kgzipfilter notices that zlib didn't read all the data, it tries to
> re-init the stream and read the rest of the buffer. This is tested by
> the unit test from Sune. The case where there's more than two streams
> available in the current buffer is tested in a unit test added
> separately.
> 
> If the split between the streams falls right between two buffers, we
> need KCompressionDevice to notice that there's data left and try to
> continue decompressing. This is easy to test by setting BUFFER_SIZE to
> the size of the first stream in the unit test data (28 bytes).
> 
> 
> Diffs
> -
> 
>   autotests/kfiltertest.cpp 4408dbe 
>   src/kcompressiondevice.cpp d6ecebd 
>   src/kgzipfilter.cpp 27a6d99 
> 
> Diff: https://git.reviewboard.kde.org/r/128369/diff/
> 
> 
> Testing
> ---
> 
> The unit test now passes.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 128369: Handle multiple gzip streams in a single file

2016-07-05 Thread David Faure 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128369/#review97114
---


Ship it!




Ship It!

- David Faure


On July 4, 2016, 10:33 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128369/
> ---
> 
> (Updated July 4, 2016, 10:33 p.m.)
> 
> 
> Review request for KDE Frameworks and Sune Vuorela.
> 
> 
> Repository: karchive
> 
> 
> Description
> ---
> 
> Handle multiple gzip streams
> 
> If kgzipfilter notices that zlib didn't read all the data, it tries to
> re-init the stream and read the rest of the buffer. This is tested by
> the unit test from Sune. The case where there's more than two streams
> available in the current buffer is tested in a unit test added
> separately.
> 
> If the split between the streams falls right between two buffers, we
> need KCompressionDevice to notice that there's data left and try to
> continue decompressing. This is easy to test by setting BUFFER_SIZE to
> the size of the first stream in the unit test data (28 bytes).
> 
> 
> Diffs
> -
> 
>   autotests/kfiltertest.cpp 4408dbe 
>   src/kcompressiondevice.cpp d6ecebd 
>   src/kgzipfilter.cpp 27a6d99 
> 
> Diff: https://git.reviewboard.kde.org/r/128369/diff/
> 
> 
> Testing
> ---
> 
> The unit test now passes.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 128369: Handle multiple gzip streams in a single file

2016-07-05 Thread Martin Tobias Holmedahl Sandsmark 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128369/
---

(Updated July 5, 2016, 11:25 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Sune Vuorela.


Changes
---

Submitted with commit 853496897f5216909337ed8711e1b8391ae1b6a3 by Martin T. H. 
Sandsmark to branch master.


Repository: karchive


Description
---

Handle multiple gzip streams

If kgzipfilter notices that zlib didn't read all the data, it tries to
re-init the stream and read the rest of the buffer. This is tested by
the unit test from Sune. The case where there's more than two streams
available in the current buffer is tested in a unit test added
separately.

If the split between the streams falls right between two buffers, we
need KCompressionDevice to notice that there's data left and try to
continue decompressing. This is easy to test by setting BUFFER_SIZE to
the size of the first stream in the unit test data (28 bytes).


Diffs
-

  autotests/kfiltertest.cpp 4408dbe 
  src/kcompressiondevice.cpp d6ecebd 
  src/kgzipfilter.cpp 27a6d99 

Diff: https://git.reviewboard.kde.org/r/128369/diff/


Testing
---

The unit test now passes.


Thanks,

Martin Tobias Holmedahl Sandsmark

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127813: Reduce string modifications when calling translatePath

2016-07-05 Thread David Faure 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127813/#review97136
---


Ship it!




Looks good to me, thanks. Extending the unittests (KConfigTest::testPath I 
think) to test for the file:///file%23with%23hash case would be even better, of 
course.

- David Faure


On July 4, 2016, 10:36 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127813/
> ---
> 
> (Updated July 4, 2016, 10:36 p.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Don't drop the file: prefix twice in every run.
> Use appropriate API on paths rather than string handling, when possible.
> 
> 
> Diffs
> -
> 
>   src/core/kconfiggroup.cpp 39d2441 
> 
> Diff: https://git.reviewboard.kde.org/r/127813/diff/
> 
> 
> Testing
> ---
> 
> Tests pass, apps work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Where to put kwayland integration plugins

2016-07-05 Thread Albert Astals Cid 
El dissabte, 2 de juliol de 2016, a les 15:25:39 CEST, Martin Graesslin va 
escriure:
> On Saturday, July 2, 2016 11:41:11 AM CEST David Faure wrote:
> > On lundi 6 juin 2016 11:26:58 CEST Aleix Pol wrote:
> > > On Mon, Jun 6, 2016 at 8:32 AM, Martin Graesslin 
> 
> wrote:
> > > > Hi framework-developers,
> > > > 
> > > > in Plasma we have a repository called kwayland-integration. It
> > > > provides
> > > > a
> > > > plugin for:
> > > > * KWindowSystem
> > > > * KIdleTime
> > > > 
> > > > using the KWayland client API. Thus it makes these frameworks
> > > > functional
> > > > on
> > > > windowing system Wayland.
> > > > 
> > > > I want to move the plugins into frameworks and are wondering how to do
> > > > that
> > > > and where to move them to.
> > > > 
> > > > I see the following options:
> > > > 1. Integrate directly into kwindowsystem and kidletime
> > > > 2. Merge them into frameworks-integration
> > > > 3. Create a new framework kwayland-integration
> > > > 4. create a new framework "kwindowsystem-wayland" and
> > > > "kidletime-wayland"
> > > > 
> > > > Option 1 is I think an absolute no-no as it would turn kwindowsystem
> > > > and
> > > > kidletime from tier 1 to tier 2 and that would affect a huge amount of
> > > > other frameworks. For everything which is not in tier 1, I think it's
> > > > the
> > > > way to go.
> > > > 
> > > > Option 2 is something I don't want to do as that would combine
> > > > windowing
> > > > system integration code with random other stuff and would result in
> > > > weird
> > > > dependencies. (To use KIdleTime on Wayland one needs X11 and Phonon?)
> > > > 
> > > > The same actually also applies to Option 3, though it is just two
> > > > frameworks and all dependencies would be tier 1.
> > > > 
> > > > So personally I think option 3 or option 4 are the way to go.
> > > > 
> > > > What do you think? Better ideas?
> > > 
> > > I agree ideally it's 3 or 4. Where do you have it now? Are they
> > > already separate repositories? If so, just moving them to frameworks
> > > could make sense.
> > > 
> > > In Purpose I have a similar problem (and I know the discussion is also
> > > relevant for other frameworks). We usually don't want plugins to raise
> > > the tier of your base framework, but you still need them to be
> > > distributed together and either way you need to make sure the plugins
> > > will be available when the system is deployed (which is actually much
> > > easier in option 1).
> > 
> > The alternative is to use option 1 with a cmake option to disable the
> > kwayland-based plugin. This offers benefits because
> > - on systems with wayland enabled, you are sure the plugin is present (no
> > bug report like "idle detection doesn't work" (because of a missing
> > package)) - on systems where dependencies should be kept to a minimum
> > (e.g.
> > an X11-based embedded system) one can easily compile without the kwayland
> > dependency
> > 
> > Application developers using KIdleTime or KWindowSystem do expect it to
> > work on all platforms where the application works, so surely a dependency
> > on kwayland can't be a problem on wayland.
> 
> That's actually not the case. Both KIdleTime and KWindowSystem (runtime)
> depend on additional Wayland protocols currently only provided in KWin/
> Wayland. That means your KIdleTime based application won't work in e.g.
> GNOME.
> 
> Given that the real world benefit of option 1 is not that large.

I'm confused by this, rsibreak uses "KIdleTime::instance()->idleTime()" when 
you say it won't work in GNOME, you mean "GNOME Wayland" or "any GNOME at 
all"?

Cheers,
  Albert

> 
> Cheers
> Martin


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KMessageBox runtime dependency on FrameworkIntegrationPlugin makes it useless

2016-07-05 Thread Albert Astals Cid 
El dissabte, 2 de juliol de 2016, a les 12:23:31 CEST, David Faure va 
escriure:
> On mercredi 15 juin 2016 00:33:40 CEST Albert Astals Cid wrote:
> > El dimarts, 14 de juny de 2016, a les 10:57:19 CEST, David Faure va
> 
> escriure:
> > > On mardi 14 juin 2016 01:00:09 CEST Albert Astals Cid wrote:
> > > > El dilluns, 13 de juny de 2016, a les 11:27:53 CEST, David Faure va
> > > 
> > > escriure:
> > > > > On dimanche 12 juin 2016 22:58:34 CEST Albert Astals Cid wrote:
> > > > > > El diumenge, 12 de juny de 2016, a les 20:29:56 CEST, Christoph
> > > > > > Cullmann
> > > > > > va
> > > > > > 
> > > > > > escriure:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > > Having KMessageBox "dontShowMeAgain" feature depend on an
> > > > > > > > integration
> > > > > > > > plugin is a very bad idea.
> > > > > > > > 
> > > > > > > > Basically it means programs that use KMessageBox can never
> > > > > > > > asusme
> > > > > > > > it
> > > > > > > > will
> > > > > > > > work so basically they have to use alternative methods to have
> > > > > > > > the
> > > > > > > > "dontShowMeAgain" feature or not have it at all.
> > > > > > > > 
> > > > > > > > I understand someone thought that it was a better idea having
> > > > > > > > a
> > > > > > > > feature
> > > > > > > > that may work or not randomly that increasing the dependency
> > > > > > > > chain
> > > > > > > > of
> > > > > > > > KMessageBox, but I disagree.
> > > > > > > > 
> > > > > > > > I don't think the status quo is good at all, my program
> > > > > > > > basically
> > > > > > > > gets
> > > > > > > > a
> > > > > > > > runtime dependency that is not specified anywhere and that
> > > > > > > > makes
> > > > > > > > some
> > > > > > > > features work or not randomly.
> > > > > > > > 
> > > > > > > > The options I can see are:
> > > > > > > > * Remove the "dontShowMeAgain" feature from KMessageBox
> > > > > > > > * Make the "dontShowMeAgain" feature use QSettings (always or
> > > > > > > > if
> > > > > > > > FrameworkIntegrationPlugin is not available)
> > > > > > > > * Show a KMessageBox warning when trying to use the
> > > > > > > > "dontShowMeAgain"
> > > > > > > > feature and the FrameworkIntegrationPlugin is not available
> > > > > > > > saying
> > > > > > > > the
> > > > > > > > user to install that package if he wants to get the
> > > > > > > > functionality.
> > > > > > > 
> > > > > > > I would go for the "just use QSettings always" solution.
> > > > > > 
> > > > > > This has two problems:
> > > > > >  * Someone needs to care about a way to read the KConfig and write
> > > > > >  it
> > > > > >  to
> > > > > > 
> > > > > > QSettings so it still works for people that had already checked
> > > > > > "don't
> > > > > > show
> > > > > > me again"
> > > > > > 
> > > > > >  * There's a KMessageBox::setDontShowAgainConfig(KConfig *cfg)
> > > > > >  that
> > > > > >  would
> > > > > > 
> > > > > > break, this affects only the kdialog app and KIO::JobUiDelegate
> > > > > > 
> > > > > > The second is probably "workaroundable" but i'm not sure how one
> > > > > > would
> > > > > > approach the first.
> > > > > > 
> > > > > > Maybe still using the FrameworkIntegrationPlugin to check if the
> > > > > > option
> > > > > > has
> > > > > > been set and if it has and it is not in qsettings, move it there?
> > > > > > 
> > > > > > This would "fail" if the FrameworkIntegrationPlugin is not there,
> > > > > > but
> > > > > > that
> > > > > > would mean that most probably the kconfig was never ever set
> > > > > > either
> > > > > > (unless
> > > > > > you installed FrameworkIntegrationPlugin used a kmessagebox and
> > > > > > then
> > > > > > uninstalled FrameworkIntegrationPlugin that seems very corner
> > > > > > case-y).
> > > > > > 
> > > > > > I'll try to work on this in the coming days if noone disagrees.
> > > > > 
> > > > > Not sure exactly what is your final approach, from the above.
> > > > > 
> > > > > I would go for:
> > > > >   Make the "dontShowMeAgain" feature use QSettings if
> > > > >   FrameworkIntegrationPlugin is not available
> > > > > 
> > > > > i.e. as a fallback. This preserves the KConfig benefits/integration
> > > > > on
> > > > > a
> > > > > more complete install, but doesn't lead to a non-working checkbox
> > > > > when
> > > > > the
> > > > > plugin is missing.
> > > > 
> > > > That is not what i suggested, I suggested using QSettings only.
> > > > 
> > > > What's the point of using KConfig for this?
> > > 
> > > I can think of the following:
> > > - not breaking setDontShowAgainConfig(KConfig *cfg)
> > > - not breaking application code that might be setting or clearing
> > > these settings directly using KConfig (e.g. to implement "show all
> > > messageboxes again")
> > 
> > That would still not work if only using QSettings, application is again
> > only half working because we support two different places to save the
> > same value.
> 
> Right, I meant that at least these apps would keep working when the
> integrationplugin is installed. If you us

Re: Review Request 126738: Remove external dependencies from svgs

2016-07-05 Thread Andreas Kainz 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126738/#review96341
---



I made a cleanup off all breeze icons so it should now work without troubles if 
you find some please let me know. otherwise you can close the request.

thanks
Andreas

- Andreas Kainz


On Jan. 14, 2016, 1:18 vorm., Dirk Hohndel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126738/
> ---
> 
> (Updated Jan. 14, 2016, 1:18 vorm.)
> 
> 
> Review request for KDE Frameworks, Andreas Kainz and Uri Herrera.
> 
> 
> Repository: breeze-icons
> 
> 
> Description
> ---
> 
> I don't know what I'm doing here. But when using the icons in a QML app I
> get a lot of warnings like this one:
> 
> Could not resolve property : linearGradient4548
> 
> Running the svgs through inkscape and using the "clean up document"
> function results in this commit (and the warnings are gone).
> 
> This may not be the right thing to do but it would be nice to get rid of
> all these warnings when using the icons.
> 
> Signed-off-by: Dirk Hohndel 
> 
> 
> Diffs
> -
> 
>   icons/actions/24/dialog-cancel.svg a2b022a9dc7b7cf97e4c3391b87e7800355d2dcb 
>   icons/actions/24/distribute-horizontal-x.svg 
> 262fcaca937cb72d8393fd5dff9c59651367fe36 
>   icons/actions/24/document-edit.svg afb37ca9624b37d6ac5aa9c94f1b2cef620faff6 
>   icons/actions/24/document-save.svg cee0a3521deb7eb1fcb79266afaa8951f4984b20 
> 
> Diff: https://git.reviewboard.kde.org/r/126738/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dirk Hohndel
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Where to put kwayland integration plugins

2016-07-05 Thread Martin Graesslin 
On Wednesday, July 6, 2016 12:16:09 AM CEST Albert Astals Cid wrote:
> El dissabte, 2 de juliol de 2016, a les 15:25:39 CEST, Martin Graesslin va
> 
> escriure:
> > On Saturday, July 2, 2016 11:41:11 AM CEST David Faure wrote:
> > > On lundi 6 juin 2016 11:26:58 CEST Aleix Pol wrote:
> > > > On Mon, Jun 6, 2016 at 8:32 AM, Martin Graesslin 
> > 
> > wrote:
> > > > > Hi framework-developers,
> > > > > 
> > > > > in Plasma we have a repository called kwayland-integration. It
> > > > > provides
> > > > > a
> > > > > plugin for:
> > > > > * KWindowSystem
> > > > > * KIdleTime
> > > > > 
> > > > > using the KWayland client API. Thus it makes these frameworks
> > > > > functional
> > > > > on
> > > > > windowing system Wayland.
> > > > > 
> > > > > I want to move the plugins into frameworks and are wondering how to
> > > > > do
> > > > > that
> > > > > and where to move them to.
> > > > > 
> > > > > I see the following options:
> > > > > 1. Integrate directly into kwindowsystem and kidletime
> > > > > 2. Merge them into frameworks-integration
> > > > > 3. Create a new framework kwayland-integration
> > > > > 4. create a new framework "kwindowsystem-wayland" and
> > > > > "kidletime-wayland"
> > > > > 
> > > > > Option 1 is I think an absolute no-no as it would turn kwindowsystem
> > > > > and
> > > > > kidletime from tier 1 to tier 2 and that would affect a huge amount
> > > > > of
> > > > > other frameworks. For everything which is not in tier 1, I think
> > > > > it's
> > > > > the
> > > > > way to go.
> > > > > 
> > > > > Option 2 is something I don't want to do as that would combine
> > > > > windowing
> > > > > system integration code with random other stuff and would result in
> > > > > weird
> > > > > dependencies. (To use KIdleTime on Wayland one needs X11 and
> > > > > Phonon?)
> > > > > 
> > > > > The same actually also applies to Option 3, though it is just two
> > > > > frameworks and all dependencies would be tier 1.
> > > > > 
> > > > > So personally I think option 3 or option 4 are the way to go.
> > > > > 
> > > > > What do you think? Better ideas?
> > > > 
> > > > I agree ideally it's 3 or 4. Where do you have it now? Are they
> > > > already separate repositories? If so, just moving them to frameworks
> > > > could make sense.
> > > > 
> > > > In Purpose I have a similar problem (and I know the discussion is also
> > > > relevant for other frameworks). We usually don't want plugins to raise
> > > > the tier of your base framework, but you still need them to be
> > > > distributed together and either way you need to make sure the plugins
> > > > will be available when the system is deployed (which is actually much
> > > > easier in option 1).
> > > 
> > > The alternative is to use option 1 with a cmake option to disable the
> > > kwayland-based plugin. This offers benefits because
> > > - on systems with wayland enabled, you are sure the plugin is present
> > > (no
> > > bug report like "idle detection doesn't work" (because of a missing
> > > package)) - on systems where dependencies should be kept to a minimum
> > > (e.g.
> > > an X11-based embedded system) one can easily compile without the
> > > kwayland
> > > dependency
> > > 
> > > Application developers using KIdleTime or KWindowSystem do expect it to
> > > work on all platforms where the application works, so surely a
> > > dependency
> > > on kwayland can't be a problem on wayland.
> > 
> > That's actually not the case. Both KIdleTime and KWindowSystem (runtime)
> > depend on additional Wayland protocols currently only provided in KWin/
> > Wayland. That means your KIdleTime based application won't work in e.g.
> > GNOME.
> > 
> > Given that the real world benefit of option 1 is not that large.
> 
> I'm confused by this, rsibreak uses "KIdleTime::instance()->idleTime()" when
> you say it won't work in GNOME, you mean "GNOME Wayland" or "any GNOME at
> all"?

GNOME Wayland.

Cheers
Martin

signature.asc
Description: This is a digitally signed message part.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel